[PATCH net] xfrm: fix IPv6 first-fragment protocol decode

[PATCH net] 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 GRE and
ICMPv6 selector fields unset for fragmented packets, matching the old
decoder's behavior.

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 | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 9ea60c7717ff..c9c2370b07cc 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3489,14 +3489,16 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve
 		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)) {
+		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 +3509,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

Re: [PATCH net] xfrm: fix IPv6 first-fragment protocol decode

[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] xfrm: fix IPv6 first-fragment protocol decode

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (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).

- [Medium] After this patch, decode_session6() stays compatible with the
  old decoder's fragment behaviour only when the built-in C flow…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is an IPsec
  policy bypass on the same code path.

--

xfrm: fix IPv6 first-fragment protocol decode

For AF_INET6, this commit makes __xfrm_decode_session() pass
FLOW_DISSECTOR_F_PARSE_1ST_FRAG, so the flow dissector walks past an
IPv6 Fragment header to the real upper-layer protocol. It also changes
decode_session6() so the GRE key and ICMPv6 type/code are only set when
FLOW_DIS_IS_FRAGMENT is clear, which matches the old decoder.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 9ea60c7717ff8..c9c2370b07cc1 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3489,14 +3489,16 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve
>  		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)) {
> +		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;
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but it is
on the same path. This path now finds the real L4 protocol for IPv6
atomic fragments (offset 0, M=0). Could the ports and ICMPv6 type/code be
recovered as well?

An atomic fragment is a complete datagram. ipv6_frag_rcv() only moves the
transport header past the Fragment header and returns 1:

net/ipv6/reassembly.c:ipv6_frag_rcv() {
    ...
	if (!(fhdr->frag_off & htons(IP6_OFFSET | IP6_MF))) {
		/* It is not a fragmented frame */
		skb->transport_header += sizeof(struct frag_hdr);
		...
		return 1;
    ...
}

After that, udpv6_rcv()/tcp_v6_rcv() -> xfrm6_policy_check() ->
__xfrm_decode_session() -> __skb_flow_dissect() dissects the packet again
from the network header. With PARSE_1ST_FRAG, ip_proto is now correct.
The ports are still skipped, though:

net/core/flow_dissector.c:__skb_flow_dissect() {
    ...
	if (!(key_control->flags & FLOW_DIS_IS_FRAGMENT))
		__skb_flow_dissect_ports(skb, flow_dissector, target_container,
					 data, nhoff, ip_proto, hlen);
    ...
}

So flkeys.ports keeps its memset value of zero, and decode_session6()
copies zeros into fl6_sport/fl6_dport. The new gate above also leaves the
ICMPv6 type/code and the GRE key unset. The ICMPv6 type/code share storage
with the port fields that xfrm_flowi_sport()/xfrm_flowi_dport() read.

As a result, __xfrm6_selector_match() can never match a selector scoped by
port, ICMPv6 type or GRE key:

	!((xfrm_flowi_dport(fl, &fl6->uli) ^ sel->dport) & sel->dport_mask) &&

Take an inbound "proto udp dport 1701, tmpl ESP level required" policy,
with no broader policy covering that traffic and a default inbound policy
of accept. __xfrm_policy_check() then takes its !pol branch and returns 1.
Could a remote sender put plaintext UDP or TCP inside an atomic Fragment
header and have it delivered to the socket that policy is meant to
protect?

The old open-coded decoder behaved the same way (NEXTHDR_FRAGMENT set
onlyproto = 1), and the commit message keeps this on purpose. For atomic
fragments (FLOW_DIS_FIRST_FRAG with M=0), the ports are as reliable as
the protocol. Would it make sense to handle that case here?

>  
>  	fl6->flowi6_proto = flkeys->basic.ip_proto;
> @@ -3507,10 +3509,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: Medium]
Is the old fragment behaviour still kept when a BPF flow dissector is
attached?

decode_session6() copies flkeys->ports into fl6_sport/fl6_dport for every
packet. Only the GRE/ICMPv6 fields check FLOW_DIS_IS_FRAGMENT. Fragments
get zero ports only because the C dissector skips
__skb_flow_dissect_ports() when FLOW_DIS_IS_FRAGMENT is set.

When a BPF_PROG_TYPE_FLOW_DISSECTOR program is attached to init_net or to
the netns, __skb_flow_dissect() runs it instead of the C code.
bpf_flow_dissect() passes the new flag through unchanged:

net/core/flow_dissector.c:bpf_flow_dissect() {
    ...
	flow_keys->flags = flags;
    ...
}

The in-tree reference program, PROG(IPV6FR) in
tools/testing/selftests/bpf/progs/bpf_flow.c, keeps parsing first
fragments when PARSE_1ST_FRAG is set:

	return parse_ipv6_proto(skb, fragh->nexthdr);

That fills in sport/dport. __skb_flow_bpf_to_target() then copies them
into FLOW_DISSECTOR_KEY_PORTS without any fragment check:

		key_ports->src = flow_keys->sport;
		key_ports->dst = flow_keys->dport;

So with a BPF dissector, IPv6 first fragments now reach the XFRM policy
lookup with real ports. Later fragments of the same datagram get zero
ports, and so does every fragment under the C dissector.

The forward path checks fragments one at a time:
ip6_forward() -> xfrm6_policy_check(XFRM_POLICY_FWD), then
xfrm6_route_forward(). Could a port-scoped selector match the first
fragment but not the rest, giving fragments of one datagram different
policy and routing decisions?

Before this patch PARSE_1ST_FRAG was never passed, so both dissectors
stopped at the Fragment header. That conflicts with this part of the
commit message:

  "Keep GRE and ICMPv6 selector fields unset for fragmented packets,
   matching the old decoder's behavior."

Would putting the port assignment in decode_session6() under the same
FLOW_DIS_IS_FRAGMENT check used for GRE/ICMPv6 remove this dependency on
the dissector?

>  
>  	switch (family) {
>  	case AF_INET:

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