[PATCH net v3 0/2] xfrm: fix IPv6 atomic fragment policy handling

[PATCH net v3 0/2] xfrm: fix IPv6 atomic fragment policy handling

[Jérémy Jean]

This series of two patches fixes two IPv6 atomic fragment issues in
XFRM session decoding.

Patch 1 restores upper-layer protocol decoding for IPv6 first
fragments.

Patch 2 builds on Patch 1 and restores selector fields for IPv6 atomic
fragments, while leaving them unset for real fragments.

Changes in v2 after a review from Sashiko:
- Patch 1: Leave ports unset for fragments with BPF flow dissectors.
- Patch 2: Include the atomic fragment selector fix.

Changes in v3 after a revew from Sashiko:
- Patch 1: unchanged.
- Patch 2: Safely detect atomic fragments.

v1: https://lore.kernel.org/all/20261101230745.2043474-2-Jeremy.Jean@oss.cyber.gouv.fr/
Sashiko on v1: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261101230745.2043474-2-Jeremy.Jean%40oss.cyber.gouv.fr
Sashiko on v2: https://sashiko.dev/#/patchset/20261103225842.3204446-2-Jeremy.Jean%40oss.cyber.gouv.fr

Jérémy Jean (2):
  xfrm: fix IPv6 first-fragment protocol decode
  xfrm: fix selector matching for IPv6 atomic fragments

 include/net/ipv6.h      |  1 +
 net/ipv6/exthdrs_core.c |  6 ++++-
 net/xfrm/xfrm_policy.c  | 52 ++++++++++++++++++++++++++++++-----------
 3 files changed, 44 insertions(+), 15 deletions(-)

-- 
2.47.3

[PATCH net v3 1/2] xfrm: fix IPv6 first-fragment protocol decode

[Jérémy Jean]

Commit 7a0207094f1b ("xfrm: policy: replace session decode with flow
dissector") replaced XFRM session decoding with the flow dissector.
While the old IPv6 decoder kept walking past an IPv6 Fragment
extension header, the flow dissector stops unless the caller sets
FLOW_DISSECTOR_F_PARSE_1ST_FRAG. For an atomic fragment followed by
extension headers, inbound policy lookup can therefore see the header
after Fragment instead of the actual upper-layer protocol and miss
protocol selectors.

Make the dissector continue through first IPv6 fragments. Keep the
port, ICMPv6 and GRE selector fields zero for all fragments, as the
old decoder did. This also keeps the result the same when a BPF flow
dissector fills ports for a first fragment.

Fixes: 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 net/xfrm/xfrm_policy.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 9ea60c7717ff8..bda5100c9fdbf 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3480,23 +3480,29 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve
 	if (reverse) {
 		fl6->saddr = flkeys->addrs.ipv6.dst;
 		fl6->daddr = flkeys->addrs.ipv6.src;
-		fl6->fl6_sport = flkeys->ports.dst;
-		fl6->fl6_dport = flkeys->ports.src;
 	} else {
 		fl6->saddr = flkeys->addrs.ipv6.src;
 		fl6->daddr = flkeys->addrs.ipv6.dst;
-		fl6->fl6_sport = flkeys->ports.src;
-		fl6->fl6_dport = flkeys->ports.dst;
 	}
 
-	switch (flkeys->basic.ip_proto) {
-	case IPPROTO_GRE:
-		fl6->fl6_gre_key = flkeys->gre.keyid;
-		break;
-	case IPPROTO_ICMPV6:
-		fl6->fl6_icmp_type = flkeys->icmp.type;
-		fl6->fl6_icmp_code = flkeys->icmp.code;
-		break;
+	if (!(flkeys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
+		if (reverse) {
+			fl6->fl6_sport = flkeys->ports.dst;
+			fl6->fl6_dport = flkeys->ports.src;
+		} else {
+			fl6->fl6_sport = flkeys->ports.src;
+			fl6->fl6_dport = flkeys->ports.dst;
+		}
+
+		switch (flkeys->basic.ip_proto) {
+		case IPPROTO_GRE:
+			fl6->fl6_gre_key = flkeys->gre.keyid;
+			break;
+		case IPPROTO_ICMPV6:
+			fl6->fl6_icmp_type = flkeys->icmp.type;
+			fl6->fl6_icmp_code = flkeys->icmp.code;
+			break;
+		}
 	}
 
 	fl6->flowi6_proto = flkeys->basic.ip_proto;
@@ -3507,10 +3513,13 @@ int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl
 			  unsigned int family, int reverse)
 {
 	struct xfrm_flow_keys flkeys;
+	unsigned int flags = FLOW_DISSECTOR_F_STOP_AT_ENCAP;
 
 	memset(&flkeys, 0, sizeof(flkeys));
+	if (family == AF_INET6)
+		flags |= FLOW_DISSECTOR_F_PARSE_1ST_FRAG;
 	__skb_flow_dissect(net, skb, &xfrm_session_dissector, &flkeys,
-			   NULL, 0, 0, 0, FLOW_DISSECTOR_F_STOP_AT_ENCAP);
+			   NULL, 0, 0, 0, flags);
 
 	switch (family) {
 	case AF_INET:
-- 
2.47.3

[PATCH net v3 2/2] xfrm: fix selector matching for IPv6 atomic fragments

[Jérémy Jean]

Commit 7e3a42a12c4b ("xfrm6: handling fragment") stopped decoding IPv6
selector fields after a Fragment header. That is correct for real
fragments because later fragments do not carry those fields, yet an
atomic fragment is different because it contains the complete
datagram, which allows XFRM to read its ports, ICMPv6 type and code,
and GRE key. Leaving those fields zero lets a plaintext atomic
fragment miss a policy that matches on one of them.

Leave ports, ICMPv6 type and code, and GRE key unset for real
fragments. For atomic fragments, treat the packet as a complete
datagram during IPv6 session decoding and recover the ports at the
transport offset found by the flow dissector.

Have ipv6_find_hdr() report non-atomic Fragment headers while it walks
the extension header chain. Pull the base IPv6 header before calling
it so nonlinear skbs are safe, and recover selectors only when every
Fragment header it sees is atomic.

Fixes: 7e3a42a12c4b ("xfrm6: handling fragment")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 include/net/ipv6.h      |  1 +
 net/ipv6/exthdrs_core.c |  6 +++++-
 net/xfrm/xfrm_policy.c  | 17 ++++++++++++++++-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 3de07e738538f..10785721b013d 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -1110,6 +1110,7 @@ enum {
 	IP6_FH_F_FRAG		= (1 << 0),
 	IP6_FH_F_AUTH		= (1 << 1),
 	IP6_FH_F_SKIP_RH	= (1 << 2),
+	IP6_FH_F_NON_ATOMIC	= (1 << 3),
 };
 
 /* find specified header and get offset to it */
diff --git a/net/ipv6/exthdrs_core.c b/net/ipv6/exthdrs_core.c
index 4a9748338cf40..9b9e2ce72e170 100644
--- a/net/ipv6/exthdrs_core.c
+++ b/net/ipv6/exthdrs_core.c
@@ -183,7 +183,8 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
  * isn't NULL.
  *
  * if flags is not NULL and it's a fragment, then the frag flag
- * IP6_FH_F_FRAG will be set. If it's an AH header, the
+ * IP6_FH_F_FRAG will be set. IP6_FH_F_NON_ATOMIC will be set for a
+ * non-atomic Fragment header. If it's an AH header, the
  * IP6_FH_F_AUTH flag is set and target < 0, then this function will
  * stop at the AH header. If IP6_FH_F_SKIP_RH flag was passed, then this
  * function will skip all those routing headers, where segements_left was 0.
@@ -254,6 +255,9 @@ int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
 			if (!fp)
 				return -EBADMSG;
 
+			if (flags && (*fp & htons(IP6_OFFSET | IP6_MF)))
+				*flags |= IP6_FH_F_NON_ATOMIC;
+
 			_frag_off = ntohs(*fp) & ~0x7;
 			if (_frag_off) {
 				if (target < 0 &&
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index bda5100c9fdbf..679343b23df35 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3526,9 +3526,24 @@ int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl
 		decode_session4(&flkeys, fl, reverse);
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
-	case AF_INET6:
+	case AF_INET6: {
+		unsigned int offset = 0;
+		int fh_flags = 0;
+
+		if ((flkeys.control.flags & FLOW_DIS_IS_FRAGMENT) &&
+		    pskb_network_may_pull(skb, sizeof(struct ipv6hdr)) &&
+		    ipv6_find_hdr(skb, &offset, -1, NULL, &fh_flags) >= 0 &&
+		    (fh_flags & IP6_FH_F_FRAG) &&
+		    !(fh_flags & IP6_FH_F_NON_ATOMIC)) {
+			flkeys.control.flags &= ~FLOW_DIS_IS_FRAGMENT;
+			flkeys.ports.ports = skb_flow_get_ports(skb,
+								flkeys.control.thoff,
+								flkeys.basic.ip_proto,
+								NULL, 0);
+		}
 		decode_session6(&flkeys, fl, reverse);
 		break;
+	}
 #endif
 	default:
 		return -EAFNOSUPPORT;
-- 
2.47.3

Re: [PATCH net v3 0/2] xfrm: fix IPv6 atomic fragment policy handling

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v3 2/2] xfrm: fix selector matching for IPv6 atomic fragments

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix does nothing on the local-input (XFRM_POLICY_IN) path,
  which is the main bypass the commit message says it closes.
- [Medium] The new pskb_network_may_pull() call turns
  __xfrm_decode_session() from a read-only helper into one that can…

--

xfrm: fix selector matching for IPv6 atomic fragments

This patch makes __xfrm_decode_session() recover the ports, the ICMPv6
type and code, and the GRE key for IPv6 atomic fragments. Before this,
those fields were left zero for any packet with a Fragment header. It
adds an IP6_FH_F_NON_ATOMIC flag to ipv6_find_hdr() so the caller can
tell whether every Fragment header in the chain is atomic, and it pulls
the base IPv6 header before calling ipv6_find_hdr().

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 4f6a5b81e3b5a..b7deeb03276e4 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3530,9 +3530,24 @@ int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl
>  		decode_session4(&flkeys, fl, reverse);
>  		break;
>  #if IS_ENABLED(CONFIG_IPV6)
> -	case AF_INET6:
> +	case AF_INET6: {
> +		unsigned int offset = 0;
> +		int fh_flags = 0;
> +
> +		if ((flkeys.control.flags & FLOW_DIS_IS_FRAGMENT) &&
> +		    pskb_network_may_pull(skb, sizeof(struct ipv6hdr)) &&

[Severity: High]
Can this check ever succeed on the local input path?

When xfrm6_policy_check(XFRM_POLICY_IN) runs for a locally delivered
packet, skb->data already points at the transport header.
ip6_protocol_deliver_rcu() does:

    if (!pskb_pull(skb, skb_transport_offset(skb)))
        goto discard;
    ...
        if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {

tcp_v6_rcv() and the UDP and ICMPv6 receive paths run their own IN checks
from the same position.

ipv6_frag_rcv() does not reassemble an atomic fragment. It only moves the
transport header forward:

    if (!(fhdr->frag_off & htons(IP6_OFFSET | IP6_MF))) {
        /* It is not a fragmented frame */
        skb->transport_header += sizeof(struct frag_hdr);

So skb_network_offset(skb) is negative here: -48 for a plain atomic
fragment carrying TCP. pskb_network_may_pull_reason() then computes:

    return pskb_may_pull_reason(skb, skb_network_offset(skb) + len);

Because len is unsigned, -48 + 40 wraps to 0xFFFFFFF8.
pskb_may_pull_reason() then returns SKB_DROP_REASON_PKT_TOO_SMALL from
the len > skb->len check.

The && chain stops at that point and FLOW_DIS_IS_FRAGMENT stays set.
decode_session6() then still leaves the ports, the ICMPv6 type and code,
and the GRE key at zero.

Wouldn't a plaintext atomic fragment sent to a local TCP, UDP or ICMPv6
endpoint still miss an IN policy that selects on those fields? If so,
only the FWD and OUT directions actually get the fix.

With CONFIG_DEBUG_NET, wouldn't each such packet also hit
DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in pskb_may_pull_reason()? One
remote packet would then be enough to cause a WARN, or a panic with
panic_on_warn set.

Instead of pskb_network_may_pull(), could the base header be read with
skb_header_pointer() at skb_network_offset(skb)? Another option would be
to pass an explicit offset to ipv6_find_hdr().

[Severity: Medium]
Could this pull leave stale header pointers in callers of
xfrm6_policy_check()?

Before this patch, __xfrm_decode_session() only read the skb through the
flow dissector, and the dissector uses skb_header_pointer().

pskb_may_pull_reason() calls skb_might_realloc() before any length check.
Under CONFIG_FAIL_SKB_REALLOC that does
pskb_expand_head(skb, 0, 0, GFP_ATOMIC). __pskb_pull_tail() can also call
pskb_expand_head() when the base header is not linear.

Several callers keep header pointers across the policy check. For
example, tcp_v6_rcv() does:

    th = (const struct tcphdr *)skb->data;
    hdr = ipv6_hdr(skb);
    ...
    if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb))
        goto discard_it;

    tcp_v6_fill_cb(skb, hdr, th);

The TIME_WAIT path in tcp_v6_rcv() follows the same pattern, and
tcp_inbound_hash() is passed &hdr->saddr and &hdr->daddr. ip6_forward()
saves hdr on entry and reads hdr->hop_limit after
xfrm6_policy_check(XFRM_POLICY_FWD).

If the head is reallocated here, wouldn't those reads touch the freed
head?

A concrete trigger was only found with CONFIG_FAIL_SKB_REALLOC.
ip6_rcv_core() already makes the base header linear for forwarded
packets. On local input, the wrapped length fails before
__pskb_pull_tail() is reached.

The pull seems to exist only to protect the ipv6_hdr(skb)->nexthdr read
at the start of ipv6_find_hdr(). Would a skb_header_pointer() read that
doesn't modify the skb, or an explicit start offset, cover that read?

> +		    ipv6_find_hdr(skb, &offset, -1, NULL, &fh_flags) >= 0 &&
> +		    (fh_flags & IP6_FH_F_FRAG) &&
> +		    !(fh_flags & IP6_FH_F_NON_ATOMIC)) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106202910.723098-2-Jeremy.Jean%40oss.cyber.gouv.fr