lkml.org 
[lkml]   [2022]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v16 02/12] bpf: Move dynptr type check to is_dynptr_type_expected()
    On Mon, 5 Sept 2022 at 16:34, 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). It will be useful for example for BTF, to
    > restrict which dynamic pointer types can be passed to kfuncs, as initially
    > only the local type will be supported.
    >
    > Also, splitting makes the code more readable, since checking the dynamic
    > pointer type is not necessarily related to validity and initialization.
    >
    > 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).
    >
    > Cc: Joanne Koong <joannelkoong@gmail.com>
    > Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
    > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
    > ---

    I'm not sure the split is _really_ needed, but I don't feel strongly
    about it and defer to Joanne and others.
    Otherwise,
    Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.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 068b20ed34d2..10b3c0a81d09 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);
    > + enum bpf_dynptr_type dynptr_type;
    > + int spi = get_spi(reg->off);
    > +
    > /* 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
    >

    \
     
     \ /
      Last update: 2022-09-06 04:33    [W:2.513 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site