Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220830161716.754078-1-roberto.sassu@huaweicloud.com> <20220830161716.754078-3-roberto.sassu@huaweicloud.com>
In-Reply-To: <20220830161716.754078-3-roberto.sassu@huaweicloud.com>
From:   Joanne Koong <joannelkoong@gmail.com>
Date:   Tue, 30 Aug 2022 09:46:41 -0700
Message-ID: <CAJnrk1bL2MSN81ORrkm9JcFQh3qsJ1jVGXEycSjyhk+Jv_Bz2Q@mail.gmail.com>
Subject: Re: [PATCH v14 02/12] bpf: Move dynptr type check to is_dynptr_type_expected()
To:     Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, mykolal@fb.com,
        dhowells@redhat.com, jarkko@kernel.org, rostedt@goodmis.org,
        mingo@redhat.com, paul@paul-moore.com, jmorris@namei.org,
        serge@hallyn.com, shuah@kernel.org, bpf@vger.kernel.org,
        keyrings@vger.kernel.org, linux-security-module@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        deso@posteo.net, memxor@gmail.com,
        Roberto Sassu <roberto.sassu@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 30, 2022 at 9:18 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Move dynptr type check to is_dynptr_type_expected() from
> is_dynptr_reg_valid_init(), so that callers can better determine the cause
> of a negative result (dynamic pointer not valid/initialized, dynamic
> pointer of the wrong type).
>
> Also, splitting makes the code more readable, since checking the dynamic
> pointer type is not necessarily related to validity and initialization.

I think it'd be helpful to also include that btf will be using these
functions, which seems like the main motivation behind why this change
is needed.

>
> Split the validity/initialization and dynamic pointer type check also in
> the verifier, and adjust the expected error message in the test (a test for
> an unexpected dynptr type passed to a helper cannot be added due to missing
> suitable helpers, but this case has been tested manually).

The bpf_ringbuf_submit_dynptr() and bpf_ringbuf_discard_dynptr()
helpers take in only ringbuf-type dynptrs, so either of these would
work for testing the case where an incorrect dynptr type is passed in
:)

>
> Cc: Joanne Koong <joannelkoong@gmail.com>
> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  kernel/bpf/verifier.c                         | 35 ++++++++++++++-----
>  .../testing/selftests/bpf/prog_tests/dynptr.c |  2 +-
>  2 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0194a36d0b36..1b913db252a3 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -779,8 +779,8 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
>         return true;
>  }
>
> -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> -                                    enum bpf_arg_type arg_type)
> +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> +                                    struct bpf_reg_state *reg)
>  {
>         struct bpf_func_state *state = func(env, reg);
>         int spi = get_spi(reg->off);
> @@ -796,11 +796,24 @@ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_re
>                         return false;
>         }
>
> +       return true;
> +}
> +
> +static bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> +                                   struct bpf_reg_state *reg,
> +                                   enum bpf_arg_type arg_type)
> +{
> +       struct bpf_func_state *state = func(env, reg);
> +       int spi = get_spi(reg->off);
> +       enum bpf_dynptr_type dynptr_type;

nit: the above 2 lines should be swapped to maintain reverse christmas
tree order of declarations

> +
>         /* ARG_PTR_TO_DYNPTR takes any type of dynptr */
>         if (arg_type == ARG_PTR_TO_DYNPTR)
>                 return true;
>
> -       return state->stack[spi].spilled_ptr.dynptr.type == arg_to_dynptr_type(arg_type);
> +       dynptr_type = arg_to_dynptr_type(arg_type);
> +
> +       return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
>  }
>
>  /* The reg state of a pointer or a bounded scalar was saved when
> @@ -6050,21 +6063,27 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                         }
>
>                         meta->uninit_dynptr_regno = regno;
> -               } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> +               } else if (!is_dynptr_reg_valid_init(env, reg)) {
> +                       verbose(env,
> +                               "Expected an initialized dynptr as arg #%d\n",
> +                               arg + 1);
> +                       return -EINVAL;
> +               } else if (!is_dynptr_type_expected(env, reg, arg_type)) {
>                         const char *err_extra = "";
>
>                         switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
>                         case DYNPTR_TYPE_LOCAL:
> -                               err_extra = "local ";
> +                               err_extra = "local";
>                                 break;
>                         case DYNPTR_TYPE_RINGBUF:
> -                               err_extra = "ringbuf ";
> +                               err_extra = "ringbuf";
>                                 break;
>                         default:
> +                               err_extra = "<unknown>";
>                                 break;
>                         }
> -
> -                       verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> +                       verbose(env,
> +                               "Expected a dynptr of type %s as arg #%d\n",
>                                 err_extra, arg + 1);
>                         return -EINVAL;
>                 }
> diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> index bcf80b9f7c27..8fc4e6c02bfd 100644
> --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
> +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> @@ -30,7 +30,7 @@ static struct {
>         {"invalid_helper2", "Expected an initialized dynptr as arg #3"},
>         {"invalid_write1", "Expected an initialized dynptr as arg #1"},
>         {"invalid_write2", "Expected an initialized dynptr as arg #3"},
> -       {"invalid_write3", "Expected an initialized ringbuf dynptr as arg #1"},
> +       {"invalid_write3", "Expected an initialized dynptr as arg #1"},
>         {"invalid_write4", "arg 1 is an unacquired reference"},
>         {"invalid_read1", "invalid read from stack"},
>         {"invalid_read2", "cannot pass in dynptr at an offset"},
> --
> 2.25.1
>