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: <CAADnVQJ6NPGuY=c8kbpX_nLYq4oOxOBAxbDPFLuw+yr4WrQQOQ@mail.gmail.com>
Date: Thu, 3 Apr 2025 07:24:58 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: 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

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.