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

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Oct 25, 2026 at 08:40:57PM +0100, Eric Dumazet wrote:
> Le dim. 25 oct. 2026 à 17:51, Michael S. Tsirkin <mst@redhat.com> a écrit :
> >
> > On Sun, Oct 25, 2026 at 04:16:44AM +0000, netdev-bot+sashiko@kernel.org 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.


OK, do you want to propose a patch, or need me to look into it?
And are you open to patching also all known consumers,
in case we miss a producer, or only the producers?

-- 
MST