On Thu, Apr 3, 2025 at 5:27 PM Jiayuan Chen jiayuan.chen@linux.dev wrote:
April 3, 2025 at 22:24, "Alexei Starovoitov" alexei.starovoitov@gmail.com wrote:
On Sun, Mar 30, 2025 at 8:27 PM Jiayuan Chen jiayuan.chen@linux.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@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/00000000000067f65105edbd295d@google.com/T/
Signed-off-by: Jiayuan Chen jiayuan.chen@linux.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.
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).