Re: [PATCH net 1/1] seg6: validate state in netfilter continuations
flat view
From: Andrea Mayer <andrea.mayer@uniroma2.it>
Date: 2026-10-03 17:32:47
On Thu, 1 Oct 2026 02:08:53 +0800 Ren Wei [off-list ref] wrote:
From: Zixuan Chai <redacted>
Netfilter hooks can drop or replace the dst while an SRv6 packet is
queued for continuation. The seg6local callbacks must not assume that
skb_dst() still carries the state for the route being processed.
Validate the destination and SEG6_LOCAL state before using it in the
End.DX4/End.DX6 continuations and seg6_local_input_core(). The
seg6_iptunnel continuations must also resolve the SEG6 state beneath
an XFRM dst and hold a reference while processing the SRH.
Fixes: 7a3f5b0de364 ("netfilter: add netfilter hooks to SRv6 data plane")
Cc: stable@vger.kernel.org
Reported-by: Vega <redacted>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260720204430.1886091-1-xmei5@asu.edu/ (local)
Assisted-by: LLM
Signed-off-by: Zixuan Chai <redacted>
Signed-off-by: Ren Wei <redacted>Hi, Thanks for the patch. A similar fix was posted by Xiang Mei in July [1]. It got review comments and no new version yet. Xiang, are you still working on it? Xiang's v2 1/2 makes the same seg6_local.c changes as this patch. On that patch I suggested adding unlikely() to the new checks, as in patch 2/2. The same applies here. For net, I would only check the state and drop the packet, as Xiang's v2 does, with the limit already discussed on v1 and v2 (the check is on the type, not on the instance). That check alone stops the crash, also when SNAT and an XFRM policy replace the dst. Looking through the XFRM dst is not needed for this fix. The cover letter has a reproducer, but the commit message, which stays in the git log, should say how the crash was triggered and how the fix was tested.
quoted hunk ↗ jump to hunk
[snip]diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index 61c6a27bf202..e3d36fb0f290 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c[snip]@@ -60,6 +62,26 @@ 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(struct dst_entry *dst) +{ + dst = xfrm_dst_path(dst); + return dst->lwtstate; +} + +static struct lwtunnel_state *seg6_lwt_state_get(struct sk_buff *skb) +{ + struct lwtunnel_state *lwtst; + + if (!skb_valid_dst(skb)) + return NULL; + + lwtst = seg6_lwt_state(skb_dst(skb)); + if (!lwtst || lwtst->type != LWTUNNEL_ENCAP_SEG6) + return NULL; + + return lwtstate_get(lwtst);
The route that carries the lwtstate holds a reference on it, and drops it only after an RCU grace period. Which path needs the one taken by lwtstate_get()? Sashiko asks the same question [2].
quoted hunk ↗ jump to hunk
[snip]@@ -557,18 +577,16 @@ 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. - */ - lwtst = orig_dst->lwtstate; + lwtst = seg6_lwt_state_get(skb); + if (!lwtst) { + err = -EINVAL; + goto drop; + }
This comment explains why the lwtstate address is saved before ip6_route_input() or skb_dst_drop() and compared after them. This still holds after the patch: seg6_input_route() calls one of them between the save and the comparison. I don't see a reason to remove the comment here.
[snip]
[1] https://lore.kernel.org/all/20260728215448.1543553-1-xmei5@asu.edu/ (local) [2] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/8e22f1e0a04d4a48bd990872490e21bfc61f9223.1790748418.git.petalzu987@gmail.com Ciao, Andrea