Re: [PATCH net] skbuff: keep accesses through unset transport header in bounds

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

On Sun, Oct 25, 2026 at 09:18:14AM +0100, Eric Dumazet wrote:
> Le sam. 24 oct. 2026 à 14:56, Michael S. Tsirkin <mst@redhat.com> a écrit :
> >
> > Commit 2170a1f09148 ("net: no longer reset transport_header in
> > __netif_receive_skb_core()") stopped the receive path from pretending
> > that an unset transport header starts at the network header - but only
> > on CONFIG_DEBUG_NET=y builds, the idea being that fuzzers and CI could
> > catch stacks reading a transport header where none was ever established.
> > And the plan is to drop this for release as well.
> >
> > Indeed, under KASAN, an access through the unset offset produces a
> > slab-out-of-bounds splat, which is how these bugs are usually found.
> > But a regular debug build has no such safety net: the accesses land at
> > skb->head + 0xffff, some 64K past the packet data.
> >
> > I don't think release builds are exposed to oob in this way, but I want
> > to make the stack a bit more robust if yes.
> 
> 
> This is not a net candidate: there is no bug in release builds, since
> __netif_receive_skb_core() still resets the transport header when
> CONFIG_DEBUG_NET is not set. The Fixes: tag points to a commit doing
> exactly what it was meant to do.

Good point.

> 
> >  While I appreciate a nice
> > KASAN splat as much as the next developer, making even a debug kernel do
> > oob accesses was never the point of the exercise.
> >
> > Make skb_transport_header(), and its inner variant, verify and WARN,
> > then fall back to the network header position when the transport header
> > was not set. This way, accesses stay within the packet.
> 
> This adds a test and a conditional branch in a very hot helper
> (tcp_hdr(), udp_hdr(), skb_transport_offset()...), inlined in hundreds
> of places, for all builds, to protect CONFIG_DEBUG_NET=y builds only.

Good point. I'll put this in ifdef CONFIG_DEBUG_NET then?

> Falling back to the network header is also what 2170a1f09148 wanted
> to stop doing: pretending transport and network headers are the same
> is wrong, and silently hides the bug after the first WARN_ON_ONCE().
> KASAN would no longer report anything.

But the bug already manifested with a warning, why do we want
also the kasan splat much later? And why do we want memory
silently corrupted on non KASAN builds with CONFIG_DEBUG_NET?


> >
> > An extra benefit is that release and debug kernels now behave
> > identically if an unset transport header is ever reached,
> > avoids heisenbugs.
> >
> 
> Not really, the fallback path is only reachable on CONFIG_DEBUG_NET=y
> builds.

Yes, this is what I referred to as the heisenbug: release behaves one
way, debug another, so e.g. if there's a bug only with header set,
it will not reproduce. I guess it's not a concern then.

> 
> If you have an actual report (syzbot, KASAN, or a stack trace from a
> debug build), please share it, and we will fix the caller that uses
> an unset transport header. That is the whole point of the debug check.
> 
> pw-bot: rejected
> 
> > Fixes: 2170a1f09148 ("net: no longer reset transport_header in __netif_receive_skb_core()")
> > Cc: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Co-Authored-By: Claude Code <noreply@anthropic.com>
> > ---
> >  include/linux/skbuff.h | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index a3ff380d43f0..cbfeb17d86f7 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -2993,6 +2993,12 @@ static inline void skb_reset_mac_len(struct sk_buff *skb)
> >  static inline unsigned char *skb_inner_transport_header(const struct sk_buff
> >                                                         *skb)
> >  {
> > +       if (unlikely(skb->inner_transport_header ==
> > +                    (typeof(skb->inner_transport_header))~0U)) {
> 
> 
> Inner offsets do not use ~0 as an 'unset' marker, they are zeroed.
> 
> > +               DEBUG_NET_WARN_ON_ONCE(1);
> > +               return skb->head + skb->inner_network_header;
> > +       }
> > +
> >         return skb->head + skb->inner_transport_header;
> >  }
> >
> > @@ -3072,7 +3078,11 @@ static inline void skb_unset_transport_header(struct sk_buff *skb)
> >
> >  static inline unsigned char *skb_transport_header(const struct sk_buff *skb)
> >  {
> > -       DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
> > +       if (unlikely(!skb_transport_header_was_set(skb))) {
> > +               DEBUG_NET_WARN_ON_ONCE(1);
> > +               return skb->head + skb->network_header;
> > +       }
> > +
> >         return skb->head + skb->transport_header;
> >  }
> >
> > --
> > MST
> >