On Thu, Apr 3, 2025 at 5:27 PM Jiayuan Chen <[email protected]> wrote: > > April 3, 2025 at 22:24, "Alexei Starovoitov" <[email protected]> > wrote: > > > > > > > On Sun, Mar 30, 2025 at 8:27 PM Jiayuan Chen <[email protected]> wrote: > > > > > > > > The device allocates an skb, it additionally allocates a prepad size > > > > > > (usually equal to NET_SKB_PAD or XDP_PACKET_HEADROOM) but leaves it > > > > > > uninitialized. > > > > > > The bpf_xdp_adjust_head function moves skb->data forward, which allows > > > > > > users to access data belonging to other programs, posing a security risk. > > > > > > Reported-by: [email protected] > > > > > > Closes: > > > https://lore.kernel.org/all/[email protected]/T/ > > > > > > Signed-off-by: Jiayuan Chen <[email protected]> > > > > > > --- > > > > > > include/uapi/linux/bpf.h | 8 +++++--- > > > > > > net/core/filter.c | 5 ++++- > > > > > > tools/include/uapi/linux/bpf.h | 6 ++++-- > > > > > > 3 files changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > > > index defa5bb881f4..be01a848cbbf 100644 > > > > > > --- a/include/uapi/linux/bpf.h > > > > > > +++ b/include/uapi/linux/bpf.h > > > > > > @@ -2760,8 +2760,9 @@ union bpf_attr { > > > > > > * > > > > > > * long bpf_xdp_adjust_head(struct xdp_buff *xdp_md, int delta) > > > > > > * Description > > > > > > - * Adjust (move) *xdp_md*\ **->data** by *delta* bytes. Note that > > > > > > - * it is possible to use a negative value for *delta*. This helper > > > > > > + * Adjust (move) *xdp_md*\ **->data** by *delta* bytes. Note that > > > > > > + * it is possible to use a negative value for *delta*. If *delta* > > > > > > + * is negative, the new header will be memset to zero. This helper > > > > > > * can be used to prepare the packet for pushing or popping > > > > > > * headers. > > > > > > * > > > > > > @@ -2989,7 +2990,8 @@ union bpf_attr { > > > > > > * long bpf_xdp_adjust_meta(struct xdp_buff *xdp_md, int delta) > > > > > > * Description > > > > > > * Adjust the address pointed by *xdp_md*\ **->data_meta** by > > > > > > - * *delta* (which can be positive or negative). Note that this > > > > > > + * *delta* (which can be positive or negative). If *delta* is > > > > > > + * negative, the new meta will be memset to zero. Note that this > > > > > > * operation modifies the address stored in *xdp_md*\ **->data**, > > > > > > * so the latter must be loaded only after the helper has been > > > > > > * called. > > > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > > > > > index 46ae8eb7a03c..5f01d373b719 100644 > > > > > > --- a/net/core/filter.c > > > > > > +++ b/net/core/filter.c > > > > > > @@ -3947,6 +3947,8 @@ BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, > > > xdp, int, offset) > > > > > > if (metalen) > > > > > > memmove(xdp->data_meta + offset, > > > > > > xdp->data_meta, metalen); > > > > > > + if (offset < 0) > > > > > > + memset(data, 0, -offset); > > > > > > xdp->data_meta += offset; > > > > > > xdp->data = data; > > > > > > @@ -4239,7 +4241,8 @@ BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, > > > xdp, int, offset) > > > > > > return -EINVAL; > > > > > > if (unlikely(xdp_metalen_invalid(metalen))) > > > > > > return -EACCES; > > > > > > - > > > > > > + if (offset < 0) > > > > > > + memset(meta, 0, -offset); > > > > > > > Let's make everyone pay a performance penalty to silence > > KMSAN warning? > > I don't think it's a good trade off. > > Soft nack. > > > > It's not just about simply suppressing KMSAN warnings, but for security > considerations. > > So I'd like to confirm: currently, loading an XDP program only requires > CAP_NET_ADMIN and CAP_BPF permissions. If we consider this as a super > privilege, then even if uninitialized memory is exposed, I think it's okay, > as it's the developer's responsibility, for example, like the CVE in meta > https://vuldb.com/?id.246309.
And we fixed Katran. not the kernel. > Or I'm thinking, can we rely on the verifier to force the initialization > of the newly added packet boundary behavior, specifically for this special > case (although it won't be easy to implement).
