Re: [PATCH bpf v2 1/2] bpf, xdp: clean head/meta when expanding it

[Alexei Starovoitov]

On Thu, Apr 3, 2025 at 5:27 PM Jiayuan Chen <jiayuan.chen@xxxxxxxxx> wrote:
>
> April 3, 2025 at 22:24, "Alexei Starovoitov" <alexei.starovoitov@xxxxxxxxx> wrote:
>
>
>
> >
> > On Sun, Mar 30, 2025 at 8:27 PM Jiayuan Chen <jiayuan.chen@xxxxxxxxx> 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: syzbot+0e6ddb1ef80986bdfe64@xxxxxxxxxxxxxxxxxxxxxxxxx
> > >
> > >  Closes: https://lore.kernel.org/all/00000000000067f65105edbd295d@xxxxxxxxxx/T/
> > >
> > >  Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> > >
> > >  ---
> > >
> > >  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).