Re: [PATCH net] net: fix use-after-free in pskb_expand_head()

On Wed, Sep 2, 2026 at 11:02 AM Paolo Abeni [off-list ref] wrote:

On 9/1/26 7:00 PM, Eric Dumazet wrote:

quoted

On Tue, Sep 1, 2026 at 3:55 PM Paolo Abeni [off-list ref] wrote:

quoted

On 8/31/26 11:59 AM, Eric Dumazet wrote:

quoted

pskb_expand_head() and pskb_carve_inside_header() decide whether to drop
the old head gracefully via skb_release_data() or directly free it via
skb_free_head().

Blamed commit replaced the check against dataref == delta with skb_cloned(skb).

However, skb_cloned(skb) masks out the upper 16 bits (SKB_DATAREF_SHIFT)
of dataref using SKB_DATAREF_MASK. For an skb with nohdr == 1 or a clone
referencing a shared buffer where only one regular reference exists,
skb_cloned(skb) evaluates to false even when dataref is shared with other
skbs.

When pskb_expand_head() is called on such a buffer (e.g. during bridge
flooding, generic XDP, or VLAN tag insertion), it falsely assumes the head
is unshared and calls skb_free_head(skb) directly. This drops the underlying
head/frag reference immediately, while other clones or references still
point to it, causing a use-after-free in subsequent packet processing.

Introduce skb_dataref_bias() and skb_data_is_shared() to accurately test
if dataref is shared by checking atomic_read(&skb_shinfo(skb)->dataref) != bias.

Use skb_dataref_bias() in skb_data_unref() and skb_data_is_shared() in
pskb_expand_head() and pskb_carve_inside_header().

Fixes: 3e24591a19bb ("skb: Drop "fastpath" variable for skb_cloned check in pskb_expand_head")
Reported-by: syzbot+22c4f9a7026c86bcc3b8@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/6a954e27.4d659fcc.734b4.0046.GAE@google.com/ (local)
Reported-by: syzbot+128e9f5a0f85a51215b1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=128e9f5a0f85a51215b1
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/skbuff.h | 13 ++++++++++++-
 net/core/skbuff.c      |  4 ++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 421f6fc454511ea6c7b83d5227481840a7c4bcc0..efeef148e1ba50cbb0d480975dc6c3c59acb98c7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -1295,6 +1295,11 @@ static inline bool skb_unref(struct sk_buff *skb)
      return true;
 }

+static inline int skb_dataref_bias(const struct sk_buff *skb)
+{
+     return skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1;
+}
+
 static inline bool skb_data_unref(const struct sk_buff *skb,
                                struct skb_shared_info *shinfo)
 {

@@ -1303,7 +1308,7 @@ static inline bool skb_data_unref(const struct sk_buff *skb,
      if (!skb->cloned)
              return true;

-     bias = skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1;
+     bias = skb_dataref_bias(skb);

      if (atomic_read(&shinfo->dataref) == bias)
              smp_rmb();

@@ -2019,6 +2024,12 @@ static inline struct sk_buff *skb_get(struct sk_buff *skb)
  * If users == 1, we are the only owner and can avoid redundant atomic changes.
  */

+static inline bool skb_data_is_shared(const struct sk_buff *skb)
+{
+     return skb->cloned &&
+            atomic_read(&skb_shinfo(skb)->dataref) != skb_dataref_bias(skb);
+}
+
 /**
  *   skb_cloned - is the buffer a clone
  *   @skb: buffer to check

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 966af3beed94d0a9f0387cec70b5297eccfcf19f..f4c0645f98d3f2f0c8765403e0184769e928d7b0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -2331,7 +2331,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
       * is not we can just drop the old head and let the existing refcount
       * be since all we did is relocate the values
       */
-     if (skb_cloned(skb)) {
+     if (skb_data_is_shared(skb)) {

I'm sorry, I can't wrap my head around the above change; I have similar doubt WRT
the sashiko reported one:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831095952.3385596-1-edumazet%40google.com

that is: I can't see when the new condition yield different results from the
old one. Could you please describe exactly the bad layout and were/how it is
obtained?

Hi Paolo,

Here is the exact state layout and sequence where the old and new
conditions differ:

1. The Layout and Values

Suppose an sk_buff is marked with __skb_header_release(skb) (e.g. in TCP
or skb_gro_receive()):
- skb->nohdr = 1
- dataref is set to 1 + (1 << 16) = 0x00010001

Now a clone is created (e.g. in br_flood() via deliver_clone(), packet
socket tap, or veth):
- __skb_clone(clone, skb) sets:
  - clone->nohdr = 0 (clones always have nohdr = 0)
  - clone->cloned = 1
  - skb->cloned = 1
  - dataref becomes 0x00010002 (or remains 0x00010001 if
    __skb_header_release() was called during GRO on an already-referenced
    buffer).

If a clone or sibling drops a reference via skb_data_unref() (subtracting
bias = 1), dataref returns to 0x00010001.

This is the part that I can't follow: skb_data_unref() subtracts 1 if
and only if nohdr == 0. dataref should stay 0x00010001 only on the
"original" skb with nohdr == 1.

I feel the issue could be caused by __skb_header_release() being called
on a cloned skb, which is AFAIK illegal. I *think* veth in bridge with
GRO enabled could do that.

Perhaps adding a DEBUG_NET warning in __skb_header_release() would make
sense?

Arg... I missed that GRO can be fed with nohdr clones (from veth only I think)

We keep slowing down GRO to the point it is barely an optimization.

Adding back skb_header_release() and use it instead of __skb_header_release()
in GRO will not fix the fact that GRO messes with frags.

pw-bot: cr