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