bpf, xdp: clean adjust_{head,meta} memory when offset < 0

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

Posted by Jiayuan Chen 8 months, 3 weeks ago

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);
 	xdp->data_meta = meta;
 
 	return 0;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index defa5bb881f4..7b1871f2eccf 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2761,7 +2761,8 @@ 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
+ *		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.
-- 
2.47.1

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

Posted by Alexei Starovoitov 8 months, 2 weeks ago

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.

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

Posted by Jiayuan Chen 8 months, 2 weeks ago

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.

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).

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

Posted by Alexei Starovoitov 8 months, 2 weeks ago

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).

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

Posted by Willem de Bruijn 8 months, 2 weeks ago

Alexei Starovoitov 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.

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?

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

Posted by Alexei Starovoitov 8 months, 2 weeks ago

On Thu, Apr 3, 2025 at 7:32 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Alexei Starovoitov 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.
>
> 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?

Correct. When prog extends the headroom it is suppose to write
something in there. Extending the packet just to capture
some garbage bytes from the previous packet is dumb,
but doesn't compromise the safety of the kernel.
There were proposals to ask the verifier to track that the headroom
is actually initialized by the program, but it's pointless.
Dumb prog can write garbage in there just as well.
bpf_probe_read_kernel( from_random_addr) and store into the headroom.

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

Posted by Jesper Dangaard Brouer 8 months, 2 weeks ago


On 31/03/2025 05.23, Jiayuan Chen 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.

I find your description confusing, and it needs to be improved to avoid 
people interpenetrating this when reading the commit log in the future.

It is part of the UAPI that BPF programs access data belonging to other 
BPF programs.  It is the concept behind data_meta, e.g. that XDP set 
information in this memory and TC-BPF reads it again (chained XDP progs 
also have R/W access).  I hope/assume this is not the desired 
interpretation of your text.

I guess you want to say, that the intention is to avoid BPF programs 
accessing uninitialized data?
(... which is also what the code does at a glance)

> 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);
>   	xdp->data_meta = meta;
>   
>   	return 0;
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index defa5bb881f4..7b1871f2eccf 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2761,7 +2761,8 @@ 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
> + *		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.

[PATCH bpf v2 1/2] bpf, xdp: clean head/meta when expanding it
[PATCH bpf v2 2/2] selftests/bpf: add perf test for adjust_{head,meta}