Re: [PATCH net v3] net: gso: validate transport header before computing segment length

Le dim. 25 oct. 2026 à 17:51, Michael S. Tsirkin [off-list ref] a écrit :

quoted

On Sun, Oct 25, 2026 at 04:16:44AM +0000, netdev-bot+sashiko@kernel.org wrote:

quoted

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch is incomplete for the producer its own commit message
  names.

--

net: gso: validate transport header before computing segment length

This patch makes the GSO segment length helpers in net/core/gso.c check
the TCP header before they read it. If the transport header is unset, or
the TCP header is not in the linear data, skb_gso_validate_network_len()
and skb_gso_validate_mac_len() now report that the skb does not fit. It
also adds two helpers, skb_inner_transport_header_was_set() and
skb_transport_header_linear(), to include/linux/skbuff.h.

quoted

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d6894f463bf43..11c8b295bd260 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

[ ... ]

quoted

@@ -3250,6 +3257,29 @@ static inline int skb_transport_offset(const struct sk_buff *skb)
    return skb_transport_header(skb) - skb->data;
 }

+/* Is the (inner) transport header, as specified by
+ * skb->transport_header/inner_transport_header, in the linear data?
+ */
+static inline bool skb_transport_header_linear(const struct sk_buff *skb,
+                                          unsigned int hlen)
+{
+   int thoff;
+
+   if (!skb_transport_header_was_set(skb))
+           return false;
+
+   if (skb->encapsulation) {
+           if (!skb_inner_transport_header_was_set(skb))
+                   return false;
+
+           thoff = skb_inner_transport_offset(skb);
+   } else {
+           thoff = skb_transport_offset(skb);
+   }
+
+   return thoff >= 0 && thoff + hlen <= skb_headlen(skb);

[Severity: High]

The commit message names bpf_skb_adjust_room() as a way to leave a bad
transport header behind. It also says:

    Qdisc length estimation has carried a guard against exactly this since

Is that guard enough for the bpf_skb_adjust_room() case? In
qdisc_pkt_len_segs_init(), the only check is for an unset transport
header. Nothing there matches the thoff >= 0 check added here:

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
      if (!skb->encapsulation) {
              if (unlikely(!skb_transport_header_was_set(skb)))
                      return SKB_NOT_DROPPED_YET;
              hdr_len = skb_transport_offset(skb);
      } else {
              hdr_len = skb_inner_transport_offset(skb);
      }
      /* + transport layer */
      if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
              const struct tcphdr *th;

              if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
                      return SKB_DROP_REASON_SKB_BAD_GSO;

              th = (const struct tcphdr *)(skb->data + hdr_len);
              tlen = __tcp_hdrlen(th);
    ...
}

bpf_skb_net_hdr_pop() moves mac_header and network_header forward. It
leaves transport_header where it was, unless it was equal to
network_header:

net/core/filter.c:bpf_skb_net_hdr_pop() {
    ...
              skb->mac_header += len;
              skb->network_header += len;
              if (trans_same)
                      skb->transport_header = skb->network_header;
    ...
}

When the DECAP_L4 or DECAP_IPXIP flags remove the last tunnel GSO bit,
bpf_skb_net_shrink() also clears skb->encapsulation.

Take a locally VXLAN-encapsulated TCP TSO skb whose transport_header
points at the outer UDP header. If a tc program calls
bpf_skb_adjust_room(skb, -50, BPF_ADJ_ROOM_MAC,
BPF_F_ADJ_ROOM_DECAP_L4_UDP | ...), the transport offset ends up at -16
and encapsulation is 0.

A simpler BPF_ADJ_ROOM_NET shrink of 40 bytes on a plain Ethernet/IPv4
TSO skb gives an offset of -6.

If the program then redirects to egress with bpf_redirect(), this path
runs before any of the validators hardened in this patch:

__bpf_tx_skb()
  dev_queue_xmit()
    __dev_queue_xmit()
      qdisc_pkt_len_segs_init()

In that function hdr_len is an unsigned int, so -16 becomes 0xfffffff0.
hdr_len + sizeof(struct tcphdr) is then truncated to 4 when passed as the
unsigned int len argument of pskb_may_pull(), so the pull succeeds.

Wouldn't __tcp_hdrlen(th) then read about 4GB past skb->data on 64-bit?

Should qdisc_pkt_len_segs_init() get the same check, for example by
using skb_transport_header_linear()? If not, could the commit message
be changed so it does not say the qdisc path already guards against
this?

quoted

+}
+
 static inline u32 skb_network_header_len(const struct sk_buff *skb)
 {
    DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));

[ ... ]

Indeed. Not nice.

Eric, you seem to have strong opinions about how to fix these
bugs, so, before I spend time testing: a fix like the below, acceptable?

The report looks valid, but this patches _one_ consumer of a broken
transport_header, not its _producer_.

bpf_skb_net_hdr_pop() adjusts mac_header and network_header, but leaves
transport_header alone unless it was equal to network_header. If the
popped region covers the transport header (BPF_ADJ_ROOM_NET shrink,
or a DECAP_* shrink in BPF_ADJ_ROOM_MAC mode), we keep a stale offset.
It can be negative, or positive and pointing to the wrong bytes.

qdisc_pkt_len_segs_init() is only one reader. TSO drivers
(skb_tcp_all_headers(), tcp_hdr() in ndo_features_check()/ndo_start_xmit())
and the GSO validators would read the same stale offset.
A 'hdr_len < 0' test does not catch the positive case either.

bpf_skb_net_grow() already sets the transport header properly,
the shrink side should do the same. Something like (untested):

diff --git a/net/core/filter.c b/net/core/filter.c
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
 {
        bool trans_same = skb->transport_header == skb->network_header;
+       bool trans_popped = false;
        int ret;

+       if (!trans_same && skb_transport_header_was_set(skb)) {
+               int thoff = skb_transport_offset(skb);
+
+               trans_popped = thoff >= (int)off && thoff < (int)(off + len);
+       }
+
        /* Same here, __skb_push()/__skb_pull() pair not needed. */
        ret = bpf_skb_generic_pop(skb, off, len);
        if (likely(!ret)) {
                skb->mac_header += len;
                skb->network_header += len;
                if (trans_same)
                        skb->transport_header = skb->network_header;
+               else if (trans_popped)
+                       skb_set_transport_header(skb, off);
        }

        return ret;
 }

For the decap case, BPF folks might prefer to restore the transport header
from skb->inner_transport_header when skb->encapsulation is cleared.