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:   Sun, 3 Jul 2022 10:54:23 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...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 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, ...);

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).