From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Martin KaFai Lau <martin.lau@kernel.org>,
David Vernet <void@manifault.com>
Subject: Re: [PATCH bpf-next v1 01/13] bpf: Refactor ARG_PTR_TO_DYNPTR checks into process_dynptr_func
Date: Thu, 20 Oct 2022 06:25:50 +0530 [thread overview]
Message-ID: <20221020005550.fhty2mhdpua7ogpf@apollo> (raw)
In-Reply-To: <CAJnrk1b+cBapTLcgLk41AQFMsSFOwB6HR4Nu-Wsi3=pzkN+nfQ@mail.gmail.com>
On Thu, Oct 20, 2022 at 04:29:57AM IST, Joanne Koong wrote:
> On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > ARG_PTR_TO_DYNPTR is akin to ARG_PTR_TO_TIMER, ARG_PTR_TO_KPTR, where
> > the underlying register type is subjected to more special checks to
> > determine the type of object represented by the pointer and its state
> > consistency.
> >
> > Move dynptr checks to their own 'process_dynptr_func' function so that
> > is consistent and in-line with existing code. This also makes it easier
> > to reuse this code for kfunc handling.
> >
> > To this end, remove the dependency on bpf_call_arg_meta parameter by
> > instead taking the uninit_dynptr_regno by pointer. This is only needed
> > to be set to a valid pointer when arg_type has MEM_UNINIT.
> >
> > Then, reuse this consolidated function in kfunc dynptr handling too.
> > Note that for kfuncs, the arg_type constraint of DYNPTR_TYPE_LOCAL has
> > been lifted.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> > include/linux/bpf_verifier.h | 8 +-
> > kernel/bpf/btf.c | 17 +--
> > kernel/bpf/verifier.c | 115 ++++++++++--------
> > .../bpf/prog_tests/kfunc_dynptr_param.c | 5 +-
> > .../bpf/progs/test_kfunc_dynptr_param.c | 12 --
> > 5 files changed, 69 insertions(+), 88 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 9e1e6965f407..a33683e0618b 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -593,11 +593,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state
> > u32 regno);
> > int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > u32 regno, u32 mem_size);
> > -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > - struct bpf_reg_state *reg);
> > -bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> > - struct bpf_reg_state *reg,
> > - enum bpf_arg_type arg_type);
> > +int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > + enum bpf_arg_type arg_type, int argno,
> > + u8 *uninit_dynptr_regno);
> >
> > /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
> > static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index eba603cec2c5..1827d889e08a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6486,23 +6486,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > return -EINVAL;
> > }
> >
> > - if (!is_dynptr_reg_valid_init(env, reg)) {
> > - bpf_log(log,
> > - "arg#%d pointer type %s %s must be valid and initialized\n",
> > - i, btf_type_str(ref_t),
> > - ref_tname);
> > + if (process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR, i, NULL))
>
> I think it'd be helpful to add a bpf_log statement here that this failed
>
I left it out because process_dynptr_func itself will do the logging we were
doing here before.
> > return -EINVAL;
> > - }
> > -
> > - if (!is_dynptr_type_expected(env, reg,
> > - ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL)) {
> > - bpf_log(log,
> > - "arg#%d pointer type %s %s points to unsupported dynamic pointer type\n",
> > - i, btf_type_str(ref_t),
> > - ref_tname);
> > - return -EINVAL;
> > - }
> > -
> > continue;
> > }
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 6f6d2d511c06..31c0c999448e 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -782,8 +782,7 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> > return true;
> > }
> >
> > -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > - struct bpf_reg_state *reg)
> > +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);
> > @@ -802,9 +801,8 @@ bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > return true;
> > }
> >
> > -bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> > - struct bpf_reg_state *reg,
> > - enum bpf_arg_type arg_type)
> > +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;
> > @@ -5573,6 +5571,65 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
> > return 0;
> > }
> >
> > +int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > + enum bpf_arg_type arg_type, int argno,
>
> Do we need both regno and argno given that regno is always argno +
> BPF_REG_1 and in this function we only use the argno param for "argno
> + 1"? I think we could just pass in regno.
>
Hmm, not really. I can drop it.
> > + u8 *uninit_dynptr_regno)
>
> nit: this is personal preference, but I think it looks cleaner passing
> "struct bpf_call_arg_meta *meta" here instead of "u8
> *uninit_dynptr_regno".
>
Right, the thinking was that kfuncs could also handle MEM_UNINIT case, in both
cases the meta type is different but this could be same, but let's think about
that when/if dynptr API function is added as a kfunc.
> > +{
> > + struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> > +
> > + /* We only need to check for initialized / uninitialized helper
> > + * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the
> > + * assumption is that if it is, that a helper function
> > + * initialized the dynptr on behalf of the BPF program.
> > + */
> > + if (base_type(reg->type) == PTR_TO_DYNPTR)
> > + return 0;
> > + if (arg_type & MEM_UNINIT) {
> > + if (!is_dynptr_reg_valid_uninit(env, reg)) {
> > + verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* We only support one dynptr being uninitialized at the moment,
> > + * which is sufficient for the helper functions we have right now.
> > + */
> > + if (*uninit_dynptr_regno) {
> > + verbose(env, "verifier internal error: multiple uninitialized dynptr args\n");
> > + return -EFAULT;
> > + }
> > +
> > + *uninit_dynptr_regno = regno;
> > + } else {
> > + if (!is_dynptr_reg_valid_init(env, reg)) {
> > + verbose(env,
> > + "Expected an initialized dynptr as arg #%d\n",
> > + argno + 1);
> > + return -EINVAL;
> > + }
> > +
> > + 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";
> > + break;
> > + case DYNPTR_TYPE_RINGBUF:
> > + err_extra = "ringbuf";
> > + break;
> > + default:
> > + err_extra = "<unknown>";
> > + break;
> > + }
> > + verbose(env,
> > + "Expected a dynptr of type %s as arg #%d\n",
> > + err_extra, argno + 1);
> > + return -EINVAL;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > static bool arg_type_is_mem_size(enum bpf_arg_type type)
> > {
> > return type == ARG_CONST_SIZE ||
> > @@ -6086,52 +6143,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > err = check_mem_size_reg(env, reg, regno, true, meta);
> > break;
> > case ARG_PTR_TO_DYNPTR:
> > - /* We only need to check for initialized / uninitialized helper
> > - * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the
> > - * assumption is that if it is, that a helper function
> > - * initialized the dynptr on behalf of the BPF program.
> > - */
> > - if (base_type(reg->type) == PTR_TO_DYNPTR)
> > - break;
> > - if (arg_type & MEM_UNINIT) {
> > - if (!is_dynptr_reg_valid_uninit(env, reg)) {
> > - verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> > - return -EINVAL;
> > - }
> > -
> > - /* We only support one dynptr being uninitialized at the moment,
> > - * which is sufficient for the helper functions we have right now.
> > - */
> > - if (meta->uninit_dynptr_regno) {
> > - verbose(env, "verifier internal error: multiple uninitialized dynptr args\n");
> > - return -EFAULT;
> > - }
> > -
> > - meta->uninit_dynptr_regno = regno;
> > - } 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";
> > - break;
> > - case DYNPTR_TYPE_RINGBUF:
> > - err_extra = "ringbuf";
> > - break;
> > - default:
> > - err_extra = "<unknown>";
> > - break;
> > - }
> > - verbose(env,
> > - "Expected a dynptr of type %s as arg #%d\n",
> > - err_extra, arg + 1);
> > - return -EINVAL;
> > - }
> > + if (process_dynptr_func(env, regno, arg_type, arg, &meta->uninit_dynptr_regno))
> > + return -EACCES;
>
> process_dynptr_func could return -EFAULT so I think we should do "err
> = process_dynptr_func(...)" here instead.
>
Agreed, I'll also propagate errors from other similar named functions.
next prev parent reply other threads:[~2022-10-20 0:56 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-18 13:59 [PATCH bpf-next v1 00/13] Fixes for dynptr Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 01/13] bpf: Refactor ARG_PTR_TO_DYNPTR checks into process_dynptr_func Kumar Kartikeya Dwivedi
2022-10-18 19:45 ` David Vernet
2022-10-19 6:04 ` Kumar Kartikeya Dwivedi
2022-10-19 15:26 ` David Vernet
2022-10-19 22:59 ` Joanne Koong
2022-10-20 0:55 ` Kumar Kartikeya Dwivedi [this message]
2022-10-18 13:59 ` [PATCH bpf-next v1 02/13] bpf: Rework process_dynptr_func Kumar Kartikeya Dwivedi
2022-10-18 23:16 ` David Vernet
2022-10-19 6:18 ` Kumar Kartikeya Dwivedi
2022-10-19 16:05 ` David Vernet
2022-10-20 1:09 ` Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 03/13] bpf: Rename confusingly named RET_PTR_TO_ALLOC_MEM Kumar Kartikeya Dwivedi
2022-10-18 21:38 ` sdf
2022-10-19 6:19 ` Kumar Kartikeya Dwivedi
2022-11-07 22:35 ` Joanne Koong
2022-11-07 23:12 ` Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 04/13] bpf: Rework check_func_arg_reg_off Kumar Kartikeya Dwivedi
2022-10-18 21:55 ` sdf
2022-10-19 6:24 ` Kumar Kartikeya Dwivedi
2022-11-07 23:17 ` Joanne Koong
2022-11-08 18:22 ` Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 05/13] bpf: Fix state pruning for STACK_DYNPTR stack slots Kumar Kartikeya Dwivedi
2022-11-08 20:22 ` Joanne Koong
2022-11-09 18:39 ` Kumar Kartikeya Dwivedi
2022-11-10 0:41 ` Joanne Koong
2022-10-18 13:59 ` [PATCH bpf-next v1 06/13] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR Kumar Kartikeya Dwivedi
2022-10-19 18:52 ` Alexei Starovoitov
2022-10-20 1:04 ` Kumar Kartikeya Dwivedi
2022-10-20 2:13 ` Alexei Starovoitov
2022-10-20 2:40 ` Kumar Kartikeya Dwivedi
2022-10-20 2:56 ` Alexei Starovoitov
2022-10-20 3:23 ` Kumar Kartikeya Dwivedi
2022-10-21 0:46 ` Alexei Starovoitov
2022-10-21 1:53 ` Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 07/13] bpf: Fix partial dynptr stack slot reads/writes Kumar Kartikeya Dwivedi
2022-10-21 22:50 ` Joanne Koong
2022-10-21 22:57 ` Joanne Koong
2022-10-22 4:08 ` Kumar Kartikeya Dwivedi
2022-11-03 14:07 ` Joanne Koong
2022-11-04 22:14 ` Andrii Nakryiko
2022-11-04 23:02 ` Kumar Kartikeya Dwivedi
2022-11-04 23:08 ` Andrii Nakryiko
2022-10-18 13:59 ` [PATCH bpf-next v1 08/13] bpf: Use memmove for bpf_dynptr_{read,write} Kumar Kartikeya Dwivedi
2022-10-21 18:12 ` Joanne Koong
2022-10-18 13:59 ` [PATCH bpf-next v1 09/13] selftests/bpf: Add test for dynptr reinit in user_ringbuf callback Kumar Kartikeya Dwivedi
2022-10-19 16:59 ` David Vernet
2022-10-18 13:59 ` [PATCH bpf-next v1 10/13] selftests/bpf: Add dynptr pruning tests Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 11/13] selftests/bpf: Add dynptr var_off tests Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 12/13] selftests/bpf: Add dynptr partial slot overwrite tests Kumar Kartikeya Dwivedi
2022-10-18 13:59 ` [PATCH bpf-next v1 13/13] selftests/bpf: Add dynptr helper tests Kumar Kartikeya Dwivedi
2023-10-31 7:05 ` CVE-2023-39191 - Dynptr fixes - reg Nandhini Rengaraj
2023-10-31 7:13 ` Greg KH
2023-10-31 7:57 ` Shung-Hsi Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221020005550.fhty2mhdpua7ogpf@apollo \
--to=memxor@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=joannelkoong@gmail.com \
--cc=martin.lau@kernel.org \
--cc=void@manifault.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox