netdev - Re: [PATCH v13 bpf-next 03/10] bpf: Allow initializing dynptrs in kfuncs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJnrk1ZF5FEtXKsMEnwbLu5qr-mQ6-j9+PK2j1NEf=hLE1CCKQ@mail.gmail.com>
Date:   Mon, 6 Mar 2023 22:53:48 -0800
From:   Joanne Koong <joannelkoong@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf@...r.kernel.org, martin.lau@...nel.org, andrii@...nel.org,
        ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org,
        toke@...nel.org
Subject: Re: [PATCH v13 bpf-next 03/10] bpf: Allow initializing dynptrs in kfuncs

On Sun, Mar 5, 2023 at 11:36 PM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> On Wed, Mar 01, 2023 at 04:49:46PM CET, Joanne Koong wrote:
> > This change allows kfuncs to take in an uninitialized dynptr as a
> > parameter. Before this change, only helper functions could successfully
> > use uninitialized dynptrs. This change moves the memory access check
> > (including stack state growing and slot marking) into
> > process_dynptr_func(), which both helpers and kfuncs call into.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@...il.com>
> > ---
> >  kernel/bpf/verifier.c | 67 ++++++++++++++-----------------------------
> >  1 file changed, 22 insertions(+), 45 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index e0e00509846b..82e39fc5ed05 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -268,7 +268,6 @@ struct bpf_call_arg_meta {
> >       u32 ret_btf_id;
> >       u32 subprogno;
> >       struct btf_field *kptr_field;
> > -     u8 uninit_dynptr_regno;
> >  };
> >
> >  struct btf *btf_vmlinux;
> > @@ -6225,10 +6224,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
> >   * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument
> >   * type, and declare it as 'const struct bpf_dynptr *' in their prototype.
> >   */
> > -static int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > -                            enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta)
> > +static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
> > +                            enum bpf_arg_type arg_type)
> >  {
> >       struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> > +     int err;
> >
> >       /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
> >        * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
> > @@ -6254,23 +6254,23 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> >        *               to.
> >        */
> >       if (arg_type & MEM_UNINIT) {
> > +             int i;
> > +
> >               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;
> > +             /* we write BPF_DW bits (8 bytes) at a time */
> > +             for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) {
> > +                     err = check_mem_access(env, insn_idx, regno,
> > +                                            i, BPF_DW, BPF_WRITE, -1, false);
> > +                     if (err)
> > +                             return err;
> >               }
>
> I am not sure moving check_mem_access into process_dynptr_func is the right
> thing to do. Not sure if a problem already, but sooner or later it might be.
>
> The side effects of the call should take effect on the current state only after
> we have gone through all arguments for the helper/kfunc call. In this case we
> will now do stack access while processing the dynptr arg, which may affect the
> state of stack we see through other memory arguments coming later.
>
> I think it is better to do it after argument processing is done, similar to
> existing meta.access_size handling which is done after check_func_arg loop (for
> the same reasons).
>

Thanks for taking a look. I don't have a strong preference for either
so if you do feel strongly about doing the check_mem_access() only
after argument processing, I'm happy to change it. The
check_mem_access() call on the dyntpr will mark only the dynptr stack
slots, so I don't fully see how it may affect the state of stack
through other memory arguments coming later, but I do see your point
about keeping the logic more separated out.

> > [...]