netdev - Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs

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, 19 Oct 2022 13:22:54 -0700
From:   Joanne Koong <joannelkoong@...il.com>
To:     Martin KaFai Lau <martin.lau@...ux.dev>
Cc:     andrii@...nel.org, daniel@...earbox.net, ast@...nel.org,
        martin.lau@...nel.org, kuba@...nel.org, memxor@...il.com,
        toke@...hat.com, netdev@...r.kernel.org, kernel-team@...com,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs

On Fri, Sep 9, 2022 at 4:12 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 9/7/22 11:31 AM, Joanne Koong wrote:
> > For bpf prog types that don't support writes on skb data, the dynptr is
> > read-only (bpf_dynptr_write() will return an error and bpf_dynptr_data()
> > will return NULL; for a read-only data slice, there will be a separate
> > API bpf_dynptr_data_rdonly(), which will be added in the near future).
> >
> I just caught up on the v4 discussion about loadtime-vs-runtime error on
> write.  From a user perspective, I am not concerned on which error.
> Either way, I will quickly find out the packet header is not changed.
>
> For the dynptr init helper bpf_dynptr_from_skb(), the user does not need
> to know its skb is read-only or not and uses the same helper.  The
> verifier in this case uses its knowledge on the skb context and uses
> bpf_dynptr_from_skb_rdonly_proto or bpf_dynptr_from_skb_rdwr_proto
> accordingly.
>
> Now for the slice helper, the user needs to remember its skb is read
> only (or not) and uses bpf_dynptr_data() vs bpf_dynptr_data_rdonly()
> accordingly.  Yes, if it only needs to read, the user can always stay
> with bpf_dynptr_data_rdonly (which is not the initially supported one
> though).  However, it is still unnecessary burden and surprise to user.
> It is likely it will silently turn everything into bpf_dynptr_read()
> against the user intention. eg:
>
> if (bpf_dynptr_from_skb(skb, 0, &dynptr))
>         return 0;
> ip6h = bpf_dynptr_data(&dynptr, 0, sizeof(*ip6h));
> if (!ip6h) {
>         /* Unlikely case, in non-linear section, just bpf_dynptr_read()
>          * Oops...actually bpf_dynptr_data_rdonly() should be used.
>          */
>         bpf_dynptr_read(buf, sizeof(*ip6h), &dynptr, 0, 0);
>         ip6h = buf;
> }
>

I see your point. I agree that it'd be best if we could prevent this
burden on the user, but I think the trade-off would be that if we have
bpf_dynptr_data return data slices that are read-only and data slices
that are writable (where rd-only vs. writable is tracked by verifier),
then in the future we won't be able to support dynptrs that are
dynamically read-only (since to reject at load time, the verifier must
know statically whether the dynptr is read-only or not). I'm not sure
how likely it is that we'd run into a case where we'll need dynamic
read-only dynptrs though. What are your thoughts on this?

>
> > +     case BPF_DYNPTR_TYPE_SKB:
> > +     {
> > +             struct sk_buff *skb = ptr->data;
> > +
> > +             /* if the data is paged, the caller needs to pull it first */
> > +             if (ptr->offset + offset + len > skb->len - skb->data_len)
>
> nit. skb_headlen(skb)
>
> The patches can't be applied cleanly also. Please remember to rebase.
> eg. commit afef88e65554 ("selftests/bpf: Store BPF object files with
> .bpf.o extension") has landed on Sep 2.

I will use skb_headlen(skb) and rebase for the next iteration :)
Thanks for reviewing this!
>
>