Re: [PATCH net v5 5/8] ipv6: icmpv6: don't quote decrypted payload in icmpv6 errors

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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] Incomplete fix: plaintext from device decryption can still be
  quoted in an ICMPv6 error.
- [Low] In icmp6_send(), the two decrypted-skb suppression checks are
  ordered differently against the rate limiter, which does not match the…

--

ipv6: icmpv6: don't quote decrypted payload in icmpv6 errors

This commit stops icmp6_send() from quoting device-decrypted packets in
ICMPv6 errors. An error about a received decrypted skb is dropped before
the rate limit. An error about a locally originated decrypted skb is sent
only when its route is RTF_LOCAL.

> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index d8203ebad8827..949025d83e155 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -778,6 +778,13 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
>  
>  	np = inet6_sk(sk);
>  
> +	/* Don't send parts of a decrypted skb onto the network, see
> +	 * __icmp_send(): no error about a received skb. Before the rate
> +	 * limit, so that a suppressed error costs no credit.
> +	 */
> +	if (unlikely(skb_is_decrypted(skb) && skb->skb_iif))
> +		goto out_unlock;

[Severity: High]
Does this cover every ICMPv6 error builder that quotes the invoking packet?

nf_reject_skb_v6_unreach() in net/ipv6/netfilter/nf_reject_ipv6.c builds
its own Destination Unreachable and never goes through icmp6_send():

net/ipv6/netfilter/nf_reject_ipv6.c:nf_reject_skb_v6_unreach() {
    ...
	len = min_t(unsigned int, 1220, oldskb->len);
    ...
	skb_put_data(nskb, skb_network_header(oldskb), len);
    ...
}

It has no skb_is_decrypted() or skb_iif check. It can be reached from
netdev ingress chains and from bridge chains:

nft_reject_netdev_eval()
  nft_reject_netdev_send_v6_unreach()
    nf_reject_skb_v6_unreach()
    nft_reject_queue_xmit()
      dev_queue_xmit()

nft_reject_br_send_v6_unreach()
  nf_reject_skb_v6_unreach()
  br_forward()

mlx5 sets skb->decrypted in its RX path for kTLS and PSP, and this happens
before the ingress hook runs. A reject rule in a netdev ingress or bridge
chain that matches such traffic could then send up to 1220 bytes of the
decrypted L4 header and payload back out in cleartext.

The check in sk_validate_xmit_skb() doesn't catch this. nskb comes from
alloc_skb(), so it has decrypted == 0 and no socket.

The IPv4 helper nf_reject_skb_v4_unreach() seems to have the same gap. The
earlier "ipv4: icmp: don't quote decrypted payload in icmp errors" patch
also leaves it open.

Should these reject helpers get the same check?

>  	if (!icmpv6_xrlim_allow(sk, type, &fl6, apply_ratelimit))
>  		goto out_unlock;
>  

[ ... ]

> @@ -799,6 +806,15 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
>  	if (IS_ERR(dst))
>  		goto out_unlock;
>  
> +	/* An error about a locally originated decrypted skb is sent only if
> +	 * it is routed to a local address.
> +	 */
> +	if (unlikely(skb_is_decrypted(skb)) &&
> +	    !(dst_rt6_info(dst)->rt6i_flags & RTF_LOCAL)) {
> +		dst_release(dst);
> +		goto out_unlock;
> +	}

[Severity: Low]
Is it intended that this check runs after icmpv6_xrlim_allow()?

Suppose apply_ratelimit is true and the route device isn't loopback. By
this point icmpv6_xrlim_allow() has already used a peer token through
inet_peer_xrlim_allow() and has called icmp_global_consume(). The error is
then dropped here and nothing is sent.

The IPv4 version in __icmp_send() checks both conditions before the rate
limiter:

	if (unlikely(skb_is_decrypted(skb_in)) &&
	    (skb_in->skb_iif || !(rt->rt_flags & RTCF_LOCAL)))
		goto ende;

	/* peer icmp_ratelimit */
	if (!icmpv4_xrlim_allow(net, rt, &fl4, type, code, apply_ratelimit))

So the comment above the first check, "Before the rate limit, so that a
suppressed error costs no credit", holds for only one of the two
suppressions in this patch.

One way to hit this case is a locally originated decrypted skb from a
socket bound with IPV6_FREEBIND or IPV6_TRANSPARENT to an address that
isn't local.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111001831.3710881-1-toddfreed%40meta.com