netdev - Re: [PATCH bpf-next v5 1/8] bpf: Add support for forcing kfunc args to be referenced

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]

Date:   Wed, 6 Jul 2022 11:44:36 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf@...r.kernel.org, Yonghong Song <yhs@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org
Subject: Re: [PATCH bpf-next v5 1/8] bpf: Add support for forcing kfunc args
 to be referenced

On Sun, Jul 03, 2022 at 11:04:22AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@...il.com> wrote:
> >
> > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Fri, Jun 24, 2022 at 12:56:30AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > treat __ref suffix on argument name to imply that it must be a
> > > > referenced pointer when passed to kfunc. This is required to ensure that
> > > > kfunc that operate on some object only work on acquired pointers and not
> > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> > > > walking. Release functions need not specify such suffix on release
> > > > arguments as they are already expected to receive one referenced
> > > > argument.
> > > >
> > > > Note that we use strict type matching when a __ref suffix is present on
> > > > the argument.
> > > ...
> > > > +             /* Check if argument must be a referenced pointer, args + i has
> > > > +              * been verified to be a pointer (after skipping modifiers).
> > > > +              */
> > > > +             arg_ref = is_kfunc_arg_ref(btf, args + i);
> > > > +             if (is_kfunc && arg_ref && !reg->ref_obj_id) {
> > > > +                     bpf_log(log, "R%d must be referenced\n", regno);
> > > > +                     return -EINVAL;
> > > > +             }
> > > > +
> > >
> > > imo this suffix will be confusing to use.
> > > If I understand the intent the __ref should only be used
> > > in acquire (and other) kfuncs that also do release.
> > > Adding __ref to actual release kfunc will be a nop.
> > > It will be checked, but it's not necessary.
> > >
> > > At the end
> > > +struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct__ref)
> > > will behave like kptr_xchg with exception that kptr_xchg takes any btf_id
> > > while here it's fixed.
> > >
> > > The code:
> > >  if (rel && reg->ref_obj_id)
> > >         arg_type |= OBJ_RELEASE;
> > > should probably be updated with '|| arg_ref'
> > > to make sure reg->off == 0 ?
> > > That looks like a small bug.
> > >
> >
> > Indeed, I missed that. Thanks for catching it.
> >
> > > But stepping back... why __ref is needed ?
> > > We can add bpf_ct_insert_entry to acq and rel sets and it should work?
> > > I'm assuming you're doing the orthogonal cleanup of resolve_btfid,
> > > so we will have a single kfunc set where bpf_ct_insert_entry will
> > > have both acq and rel flags.
> > > I'm surely missing something.
> >
> > It is needed to prevent the case where someone might do:
> > ct = bpf_xdp_ct_alloc(...);
> > bpf_ct_set_timeout(ct->master, ...);
> >
> 
> A better illustration is probably bpf_xdp_ct_lookup and
> bpf_ct_change_timeout, since here the type for ct->master won't match
> with bpf_ct_set_timeout, but the point is the same.

Sorry, I'm still not following.
Didn't we make pointer walking 'untrusted' so ct->master cannot be
passed into any kfunc?

> > Or just obtain PTR_TO_BTF_ID by pointer walking and try to pass it in
> > to bpf_ct_set_timeout.
> >
> > __ref allows an argument on a non-release kfunc to have checks like a
> > release argument, i.e. refcounted, reg->off == 0 (var_off is already
> > checked to be 0), so use the original pointer that was obtained from
> > an acquire kfunc. As you noted, it isn't strictly needed on release
> > kfunc (like bpf_ct_insert_entry) because the same checks happen for it
> > anyway. But both timeout and status helpers should use it if they
> > "operate" on the acquired ct (from alloc, insert, or lookup).