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

[netdev-bot+sashiko@kernel.org]

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