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

[PATCH net v2 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.

v1: https://lore.kernel.org/all/20261101230745.2043474-2-Jeremy.Jean@oss.cyber.gouv.fr/
Sashiko: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261101230745.2043474-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

 net/xfrm/xfrm_policy.c | 53 +++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 14 deletions(-)

-- 
2.47.3

[PATCH net v2 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 v2 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.

Fixes: 7e3a42a12c4b ("xfrm6: handling fragment")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 net/xfrm/xfrm_policy.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index bda5100c9fdbf..24470dbd82639 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3526,9 +3526,25 @@ 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: {
+		struct frag_hdr _fh, *fh;
+		unsigned int offset = 0;
+
+		if ((flkeys.control.flags & FLOW_DIS_IS_FRAGMENT) &&
+		    ipv6_find_hdr(skb, &offset, NEXTHDR_FRAGMENT,
+				  NULL, NULL) == NEXTHDR_FRAGMENT) {
+			fh = skb_header_pointer(skb, offset, sizeof(_fh), &_fh);
+			if (fh && !(fh->frag_off & htons(IP6_OFFSET | IP6_MF))) {
+				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 v2 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 v2 1/2] xfrm: fix IPv6 first-fragment protocol decode

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message describes the bug only as an IPv6 atomic
  fragment followed by extension headers that is mis-classified on the…

--

xfrm: fix IPv6 first-fragment protocol decode

This commit makes __xfrm_decode_session() pass
FLOW_DISSECTOR_F_PARSE_1ST_FRAG to the flow dissector for IPv6. With that
flag, first fragments are walked through to the upper-layer protocol. It
also keeps the port, ICMPv6 and GRE selector fields in decode_session6()
zero for all fragments.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cea6842e6454d..4f6a5b81e3b5a 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c

[ ... ]

> @@ -3511,10 +3517,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);

[Severity: Low]
This isn't a bug, but does the commit message describe a narrower scope
than the code change? The message says:

    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.

The flag is set for every AF_INET6 decode. That means the change covers
all IPv6 first fragments (offset 0, with or without MF) in every policy
direction, not only inbound atomic fragments.

On a router without nf_defrag_ipv6, fragments reach ip6_forward() without
being reassembled. Both of these lookups go through
__xfrm_decode_session():

ip6_forward()
    xfrm6_policy_check(NULL, XFRM_POLICY_FWD, skb)
    xfrm6_route_forward(skb)

A non-atomic first fragment is now classified by its real L4 protocol
(for example UDP). Non-first fragments still stop early in
__skb_flow_dissect():

net/core/flow_dissector.c:__skb_flow_dissect() {
    ...
	case NEXTHDR_FRAGMENT: {
	...
		ip_proto = fh->nexthdr;

		if (!(fh->frag_off & htons(IP6_OFFSET))) {
			key_control->flags |= FLOW_DIS_FIRST_FRAG;
			if (flags & FLOW_DISSECTOR_F_PARSE_1ST_FRAG) {
				fdret = FLOW_DISSECT_RET_IPPROTO_AGAIN;
				break;
			}
		}

		fdret = FLOW_DISSECT_RET_OUT_GOOD;
    ...
}

So the first fragment of a forwarded datagram can now be matched against
FWD and OUT policies with a different protocol than its later fragments.
With a destination options header after Fragment, the first fragment
uses 17 and the later ones use 60. Before this patch, all of them used
60.

This is close to how the decoder behaved before 7a0207094f1b, so it is
not a functional regression. Could the commit message mention the
forward path and non-atomic first fragments? Stable backporters with
protocol-specific tunnel or required policies on gateways will see a
change in how forwarded fragmented traffic is handled.

Could the message also say plainly that missing protocol selectors means
plaintext traffic can bypass IPsec enforcement when the default policy
accepts?

The following patch, "xfrm: fix selector matching for IPv6 atomic
fragments", does not change this.

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

Re: [PATCH net v2 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: 2 · Medium: 0 · 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 new atomic-fragment shortcut in __xfrm_decode_session()
  (net/xfrm/xfrm_policy.c) checks only the FIRST IPv6 Fragment header,…
- [High] xfrm: out-of-bounds read via ipv6_find_hdr() in
  __xfrm_decode_session()

--

xfrm: fix selector matching for IPv6 atomic fragments

This commit changes __xfrm_decode_session() so that an IPv6 atomic
fragment (offset 0, M=0) is decoded as a complete datagram. It clears
FLOW_DIS_IS_FRAGMENT and recovers the ports at the transport offset found
by the flow dissector. Real fragments keep ports, ICMPv6 type/code and GRE
key unset.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 4f6a5b81e3b5a..5acb1e0c1b02f 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3530,9 +3530,25 @@ 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: {
> +		struct frag_hdr _fh, *fh;
> +		unsigned int offset = 0;
> +
> +		if ((flkeys.control.flags & FLOW_DIS_IS_FRAGMENT) &&
> +		    ipv6_find_hdr(skb, &offset, NEXTHDR_FRAGMENT,
> +				  NULL, NULL) == NEXTHDR_FRAGMENT) {
> +			fh = skb_header_pointer(skb, offset, sizeof(_fh), &_fh);
> +			if (fh && !(fh->frag_off & htons(IP6_OFFSET | IP6_MF))) {
> +				flkeys.control.flags &= ~FLOW_DIS_IS_FRAGMENT;
> +				flkeys.ports.ports = skb_flow_get_ports(skb,
> +									flkeys.control.thoff,
> +									flkeys.basic.ip_proto,
> +									NULL, 0);

[Severity: High]
Is it safe to call ipv6_find_hdr() here without first making sure the
IPv6 header is in the linear part of the skb?

Everything else in __xfrm_decode_session() goes through
__skb_flow_dissect(). That reads headers with __skb_header_pointer(),
so it copes with a nonlinear network header. ipv6_find_hdr() does not.
With *offset == 0 it starts with:

net/ipv6/exthdrs_core.c:ipv6_find_hdr() {
    ...
	u8 nexthdr = ipv6_hdr(skb)->nexthdr;
    ...
}

That reads skb_network_header(skb) + 6 directly. There is no
skb_header_pointer() or pskb_may_pull() in front of it.

xfrm_icmp_flow_decode() looks like a caller where the header may not
be linear. It clones the ICMPv6 error skb and does
pskb_pull(newskb, hl), which drops only the outer IPv6 and ICMPv6
headers. It then calls skb_reset_network_header(newskb) and
xfrm_decode_session_reverse().

Nothing pulls the quoted IPv6 header into the linear area. If that
header sits in page frags, skb_headlen(newskb) can be 0 at this point.
The dissector still copies the inner IPv6 and Fragment headers out of
the frags and sets FLOW_DIS_IS_FRAGMENT. The new ipv6_find_hdr() call
then reads ipv6_hdr(newskb)->nexthdr past skb_tail_pointer(newskb),
into tailroom or skb_shared_info.

This is reachable from xfrm_selector_inner_icmp_match() for an incoming
ICMPV6_DEST_UNREACH, ICMPV6_PKT_TOOBIG or ICMPV6_TIME_EXCEED message
whose quoted packet carries a Fragment header. A remote sender can
therefore trigger it. A BPF flow dissector that sets is_frag on a short
or nonlinear skb would reach the same call.

Before this patch, the AF_INET6 case never read the skb directly, so
this read is new. Could this avoid ipv6_find_hdr() and walk the
extension header chain with skb_header_pointer() only, starting at
skb_network_offset(skb)? Or is there a guarantee I am missing that the
network header is linear for every caller of __xfrm_decode_session()?
A pskb_may_pull() here would modify an skb that these paths otherwise
only read, so I am not sure that is the better option.

[Severity: High]
Can the Fragment header checked here differ from the one that thoff and
ip_proto describe?

With target == NEXTHDR_FRAGMENT, ipv6_find_hdr() stops at the first
Fragment header in the chain. The dissector runs with
FLOW_DISSECTOR_F_PARSE_1ST_FRAG, so it skips past any offset-0 Fragment
header and keeps walking:

net/core/flow_dissector.c:__skb_flow_dissect() {
    ...
	key_control->flags |= FLOW_DIS_IS_FRAGMENT;

	nhoff += sizeof(_fh);
	ip_proto = fh->nexthdr;

	if (!(fh->frag_off & htons(IP6_OFFSET))) {
		key_control->flags |= FLOW_DIS_FIRST_FRAG;
		if (flags & FLOW_DISSECTOR_F_PARSE_1ST_FRAG) {
			fdret = FLOW_DISSECT_RET_IPPROTO_AGAIN;
			break;
		}
	}

	fdret = FLOW_DISSECT_RET_OUT_GOOD;
    ...
}

It stops at the first Fragment header with a non-zero offset. At out:,
thoff = nhoff, so flkeys.control.thoff and flkeys.basic.ip_proto come
from the last Fragment header it processed.

Take a packet like this:

  IPv6 | Frag(off=0, M=0, nexthdr=44) | Frag(off=N>0, nexthdr=UDP) | payload

This is a real non-first fragment. ipv6_frag_rcv() treats it as one: it
skips the atomic header and then reassembles the inner header.

Here, though, only the outer atomic header is checked. FLOW_DIS_IS_FRAGMENT
is cleared, and skb_flow_get_ports() reads at an offset inside the fragment
payload. decode_session6() then copies those payload bytes into
fl6_sport/fl6_dport.

Forwarded fragments are not reassembled unless nf_defrag_ipv6 is loaded.
So a remote sender could get this packet matched against port-specific
selectors through ip6_forward()->xfrm6_policy_check(XFRM_POLICY_FWD) and
xfrm6_route_forward(). Before this patch, it could only match
port-wildcard selectors.

The opposite case also seems possible:

  IPv6 | Frag(0, M=0) | Frag(0, M=1) | UDP

This real first fragment would get its ports decoded. The later fragments
of the same datagram would still get ports=0.

Doesn't that bring back the per-fragment classification mismatch that
7e3a42a12c4b removed? It also seems to contradict this part of the commit
message:

  "Leave ports, ICMPv6 type and code, and GRE key unset for real
  fragments."

Would it be safer to require that every Fragment header in the chain is
atomic? Another option is to check that the header the dissector stopped
at is the one tested here.

> +			}
> +		}
>  		decode_session6(&flkeys, fl, reverse);
>  		break;
> +	}
>  #endif
>  	default:
>  		return -EAFNOSUPPORT;

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