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

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]
Message-ID: <67ee9be9db59b_138964294b7@willemb.c.googlers.com.notmuch>
Date: Thu, 03 Apr 2025 10:32:09 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>, 
 Jiayuan Chen <jiayuan.chen@...ux.dev>
Cc: bpf <bpf@...r.kernel.org>, 
 Jiayuan Chen <mrpre@....com>, 
 syzbot+0e6ddb1ef80986bdfe64@...kaller.appspotmail.com, 
 Alexei Starovoitov <ast@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 Andrii Nakryiko <andrii@...nel.org>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Eduard Zingerman <eddyz87@...il.com>, 
 Song Liu <song@...nel.org>, 
 Yonghong Song <yonghong.song@...ux.dev>, 
 John Fastabend <john.fastabend@...il.com>, 
 KP Singh <kpsingh@...nel.org>, 
 Stanislav Fomichev <sdf@...ichev.me>, 
 Hao Luo <haoluo@...gle.com>, 
 Jiri Olsa <jolsa@...nel.org>, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Simon Horman <horms@...nel.org>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 Mykola Lysenko <mykolal@...com>, 
 Shuah Khan <shuah@...nel.org>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Jason Xing <kerneljasonxing@...il.com>, 
 Anton Protopopov <aspsk@...valent.com>, 
 Abhishek Chauhan <quic_abchauha@...cinc.com>, 
 Jordan Rome <linux@...danrome.com>, 
 Martin Kelly <martin.kelly@...wdstrike.com>, 
 David Lechner <dlechner@...libre.com>, 
 LKML <linux-kernel@...r.kernel.org>, 
 Network Development <netdev@...r.kernel.org>, 
 "open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf v2 1/2] bpf, xdp: clean head/meta when expanding it

Alexei Starovoitov wrote:
> On Sun, Mar 30, 2025 at 8:27 PM Jiayuan Chen <jiayuan.chen@...ux.dev> 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@...kaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/00000000000067f65105edbd295d@google.com/T/
> > Signed-off-by: Jiayuan Chen <jiayuan.chen@...ux.dev>
> > ---
> >  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.

I also assumed that this was known when the feature was originally
introduced and left as is for performance reasons.

Might be good to have that explicit. And that it is deemed safe by
virtue of XDP requiring superuser privileges anyway. Or at least I
guess that was the thought process?