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

[netdev-bot+sashiko@kernel.org]

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