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