| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 30 Aug 2022 09:46:41 -0700
From: Joanne Koong <joannelkoong@...il.com>
To: Roberto Sassu <roberto.sassu@...weicloud.com>
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
martin.lau@...ux.dev, song@...nel.org, yhs@...com,
john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
haoluo@...gle.com, jolsa@...nel.org, mykolal@...com,
dhowells@...hat.com, jarkko@...nel.org, rostedt@...dmis.org,
mingo@...hat.com, paul@...l-moore.com, jmorris@...ei.org,
serge@...lyn.com, shuah@...nel.org, bpf@...r.kernel.org,
keyrings@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
deso@...teo.net, memxor@...il.com,
Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v14 02/12] bpf: Move dynptr type check to is_dynptr_type_expected()
On Tue, Aug 30, 2022 at 9:18 AM Roberto Sassu
<roberto.sassu@...weicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@...wei.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@...il.com>
> Cc: Kumar Kartikeya Dwivedi <memxor@...il.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.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
>
Powered by blists - more mailing lists