Re: [PATCH net v3 1/1] seg6: validate state in netfilter continuations

[Andrea Mayer <andrea.mayer@uniroma2.it>]

On Sat, 24 Oct 2026 15:23:04 +0800
Ren Wei <weir@nebusec.ai> wrote:

> Netfilter hooks can replace or drop skb_dst() before an SRv6 continuation
> resumes. The continuation callbacks then assume that the current dst still
> contains the expected lwtstate, which can lead to invalid state access.
>
> Validate the current dst and lwtstate in each NF_HOOK() okfn wrapper.
> Drop the skb when the state is missing or has the wrong type. Keep the
> direct core calls unchanged when nf_hooks_lwtunnel is off.
>
> Fixes: 7a3f5b0de364 ("netfilter: add netfilter hooks to SRv6 data plane")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260720204430.1886091-1-xmei5@asu.edu
> Assisted-by: LLM
> Signed-off-by: Zixuan Chai <petalzu987@gmail.com>
> Signed-off-by: Ren Wei <weir@nebusec.ai>
> Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Hi,

Thanks for the v3.

On top of what Paolo wrote, it would be good if the commit message also
said how the crash is triggered, what it causes, and how the fix was
tested.

> [snip]

> @@ -60,6 +61,21 @@ static inline struct seg6_lwt *seg6_lwt_lwtunnel(struct lwtunnel_state *lwt)
>  	return (struct seg6_lwt *)lwt->data;
>  }
>  
> +static struct lwtunnel_state *seg6_lwt_state_from_skb(struct sk_buff *skb)
> +{
> +	struct dst_entry *dst;
> +
> +	if (unlikely(!skb_valid_dst(skb)))
> +		return NULL;
> +
> +	dst = skb_dst(skb);
> +	if (unlikely(!dst->lwtstate ||
> +		     dst->lwtstate->type != LWTUNNEL_ENCAP_SEG6))
> +		return NULL;
> +
> +	return dst->lwtstate;
> +}

seg6_lwt_state_from_skb() and seg6_local_lwt_from_skb() return a pointer,
but the callers only test it against NULL. IMHO they could return bool,
with a name that says what they check, like skb_valid_dst() does. For
example seg6_skb_has_lwtstate() and seg6_local_skb_has_lwtstate().

> [snip]

> @@ -557,18 +573,15 @@ static int seg6_input_finish(struct net *net, struct sock *sk,
>  static int seg6_input_core(struct net *net, struct sock *sk,
>  			   struct sk_buff *skb)
>  {
> -	struct dst_entry *orig_dst = skb_dst(skb);
>  	struct dst_entry *dst = NULL;
>  	struct lwtunnel_state *lwtst;
>  	struct seg6_lwt *slwt;
>  	int err;
>  
> -	/* We cannot dereference "orig_dst" once ip6_route_input() or
> -	 * skb_dst_drop() is called. However, in order to detect a dst loop, we
> -	 * need the address of its lwtstate. So, save the address of lwtstate
> -	 * now and use it later as a comparison.
> +	/* Save the lwtstate address before replacing the dst to detect a
> +	 * dst reference loop.
>  	 */
> -	lwtst = orig_dst->lwtstate;
> +	lwtst = skb_dst(skb)->lwtstate;
>  
>  	slwt = seg6_lwt_lwtunnel(lwtst);

With the check in seg6_input_core_nf(), seg6_input_core() does not need to
change, so it can keep orig_dst and the original comment.

> [snip]

> @@ -667,12 +691,13 @@ static struct dst_entry *seg6_output_dst_lookup(struct net *net,
>  static int seg6_output_core(struct net *net, struct sock *sk,
>  			    struct sk_buff *skb)
>  {
> -	struct dst_entry *orig_dst = skb_dst(skb);
>  	struct dst_entry *dst = NULL;
> +	struct lwtunnel_state *lwtst;
>  	struct seg6_lwt *slwt;
>  	int err;
>  
> -	slwt = seg6_lwt_lwtunnel(orig_dst->lwtstate);
> +	lwtst = skb_dst(skb)->lwtstate;
> +	slwt = seg6_lwt_lwtunnel(lwtst);
>  

Same as seg6_input_core(): seg6_output_core() does not need to change
either.

> [snip]

Ciao,
Andrea