Re: [PATCH v14 02/12] bpf: Move dynptr type check to is_dynptr_type_expected()

On Tue, 2022-08-30 at 09:46 -0700, Joanne Koong wrote:

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.

Ok, added.

...

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 :)

Uhm, that didn't work.

If I initialize a local dynptr, and call bpf_ringbuf_submit_dynptr(), I get:

arg 1 is an unacquired reference

The only way to make this work was to create a custom helper, with OBJ_RELEASE removed.

...

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

Ok.

Thanks

Roberto

...

•   /* 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