| Message ID | 20220830161716.754078-3-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: Add kfuncs for PKCS#7 signature verification | expand |
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 >
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 > >
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; + /* 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"},