Re: [PATCH net] seg6: fix NULL deref in input_action_end_dx{4,6}_finish() after nf hook

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Jul 30, 2026 at 05:16:10PM +0200, Andrea Mayer wrote:
> On Tue, 28 Jul 2026 14:49:08 -0700
> Xiang Mei <xmei5@asu.edu> wrote:
> 
> > [snip]
> >
> > You are right. Following your hint, I also found seg6_local_input_core(),
> > which has the same shape. v2 is a series of two patches and covers all
> > five sites:
> >
> > seg6_local.c:
> >   seg6_local_input_core()        LOCAL_IN
> >   input_action_end_dx4_finish()  PRE_ROUTING
> >   input_action_end_dx6_finish()  PRE_ROUTING
> > seg6_iptunnel.c
> >   seg6_input_core()              POST_ROUTING
> >   seg6_output_core()             POST_ROUTING
> 
> Thanks for covering all five sites.
> 
> I ran into this pattern during the MUP v2 review [1] and have been
> working on the skb_ext approach since. My work covers these sites and
> also includes selftests for DNAT. I will post it as an RFC for
> net-next.
> 
> > [snip]
> >
> > So an skb extension really would do it. SKB_EXT_* is refcounted, rides
> > with the skb, and nf_queue leaves it alone. I want to be clear that your
> > suggestion is workable; I just did not take it in this series.
> >
> > My reasoning is scope. It means a new skb_ext type plus an lwtstate
> > reference on every seg6 packet, which feels like net-next material.
> 
> As things stand, preserving state across the hook needs new machinery,
> so type-check and drop is a reasonable fix for net.
> 
> > There is also one open question. I want your view on it, and that of
> > the other maintainers.
> >
> > Say a DNAT rule in PRE_ROUTING rewrites the inner packet's destination,
> > and End.DX4 then runs with its state preserved. Then which one is intended:
> >
> > 1) send the packet to slwt->nh4, the nexthop from the SRv6 route.
> >      This ignores the DNAT rule the admin installed.
> >
> >  2) route on the new destination instead. This ignores the nexthop the
> >      SRv6 route asked for.
> >
> > v2 does neither. It drops the packet. If you have a suggestion for
> > this, I can do it in v3.
> 
> For net, dropping is the safe choice. Before the fix, the same scenario
> panics the kernel (NULL deref), so a clean drop is not a regression.
> I will look at v2.

I would go for dropping the packet too, it is a simple fix for this crash.

> With the lwtstate preserved (e.g., the skb_ext approach), the existing
> code already handles both cases. Indeed, End.DX4 does
> nhaddr = slwt->nh4.s_addr ?: iph->daddr, so a configured nexthop takes
> precedence over the rewritten address, while an unconfigured one lets
> the DNAT destination drive the lookup. End.DX6 follows the same pattern
> through seg6_lookup_nexthop.

What is the usecase for a hook to clear lwtstate information?

> > I left the "different instance of the same type" case alone for the same
> > reason. The type test passes, and the behavior uses another route's
> > parameters. It is wrong, but it is not a memory bug, and it predates the
> > crash.
> 
> Without saving the original lwtstate before NF_HOOK, in theory there is
> no guarantee that the instance is the same after the hook returns. The
> skb_ext approach addresses this too, since it preserves the original
> lwtstate rather than re-reading it from the current dst.
> 
> [1] https://lore.kernel.org/netdev/20260516182556.66af27a9c63208435911990b@uniroma2.it/
> 
> > Thanks again,
> > Xiang
> 
> Thanks,
> 
> Ciao,
> Andrea