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

[Eric Dumazet]

Le dim. 25 oct. 2026 à 17:51, Michael S. Tsirkin <mst@xxxxxxxxxx> a écrit :
>
> On Sun, Oct 25, 2026 at 04:16:44AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> > 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.
> >
> > > 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
> >
> > [ ... ]
> >
> > > @@ -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?
> >
> > > +}
> > > +
> > >  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.