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

[Xiang Mei]

On Fri, Jul 24, 2026 at 6:11 AM Andrea Mayer <andrea.mayer@xxxxxxxxxxx> wrote:
>
> On Thu, 23 Jul 2026 10:09:49 -0700
> Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> > On Mon, 20 Jul 2026 20:44:29 +0000 Xiang Mei (Microsoft) wrote:
> > > When nf_hooks_lwtunnel is enabled, the End.DX4/End.DX6 actions dispatch
> > > the decapsulated inner packet through the NF_INET_PRE_ROUTING hook chain
> > > with input_action_end_dx{4,6}_finish() as the okfn. Both functions read
> > > the lwtunnel state via orig_dst = skb_dst(skb) and dereference
> > > orig_dst->lwtstate.
> >
> > Per Sashiko's feedback - we need to validate not only that the dst
> > is there but also that it is of the expected type, no?
> > --
> > pw-bot: cr
>
> Right, and seg6_input_core/seg6_output_core in seg6_iptunnel.c (added by
> 7a3f5b0de364) read skb_dst()->lwtstate after a NF_HOOK too, and this patch does
> not touch them. A hook can leave a valid dst whose lwtstate is NULL there
> (reproducible with SNAT and an XFRM policy), so they need fixing as well.
>

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

In v2:
  1/2 is for seg6_local.c; 2/2 is for seg6_iptunnel.c.

> seg6/seg6local behaviors carry their processing state in the route's
> lwtunnel_state. That state should be preserved across the hook, and after the
> hook skb_dst() may not carry it, or may point to a different instance of the
> same type.
>
> Thanks,
> Andrea

I think you are right about the model, and I spent some time trying to
follow it. These are the places the state could live across the hook:

  okfn argument   fixed signature, cannot change
  skb->cb         owned by IPCB/IP6CB during the chain
  out of band     stale after NF_QUEUE and a late reinject
  re-attach dst   defeats the rule that dropped it
  skb extension   works

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.

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.

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.

Thanks again,
Xiang