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] [day] [month] [year] [list]

Date:   Thu, 20 Oct 2022 12:30:19 -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 Wed, Oct 19, 2022 at 11:40 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 10/19/22 11:34 PM, Martin KaFai Lau wrote:
> > On 10/19/22 1:22 PM, Joanne Koong wrote:
> >> 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?
> >
> > Out of all dynptr helpers, bpf_dynptr_data() is pretty much the only important
> > function for header parsing because of the runtime offset.  This offset is good
> > to be tracked in runtime to avoid smart compiler getting in the way.  imo,
> > making this helper less usage surprise is important.  If the verifier can help,
> > then static checking is useful here.
> >
> > It is hard to comment without a real use case on when we want to flip a dynptr
> > to rdonly in a dynamic/runtime way.  Thus, comparing with the example like the
> > skb here, my preference is pretty obvious :)
> > Beside, a quick thought is doing this static checking now should now stop the
>
> typo: should *not* stop the... :(
>
> > dynamic rdonly flip later.  I imagine it will be a helper call like
> > bpf_dynptr_set_rdonly().  The verifier should be able to track this helper call.'
>

Great! I'll change this in v7 to have bpf_dynptr_data() be able to
return both read-writable and read-only data slices, where the rd-only
property is enforced by the verifier.