Re: [PATCH net v5 4/8] ipv4: icmp: don't quote decrypted payload in icmp errors

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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] Incomplete fix, and the commit message is wrong about which
  reject paths it covers.

--

ipv4: icmp: don't quote decrypted payload in icmp errors

This patch makes __icmp_send() skip the ICMP error when the invoking
skb has skb->decrypted set. The one exception is a locally originated
skb whose error is routed to a local address. The goal is to stop
device-decrypted kTLS or PSP data from being quoted back onto the
network in cleartext.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023234640.1459291-1-toddfreed%40meta.com

> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 90c0e22c29bea..e11008aa4cecd 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -979,6 +979,18 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
>  	if (rt->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))
>  		goto ende;
>  
> +	/* Don't send parts of a decrypted skb onto the network. A received skb
> +	 * was decrypted by the device, and the transport header may have been
> +	 * encrypted. The different offloads can't be distinguished in all cases
> +	 * so simply don't send the error.
> +	 *
> +	 * A locally originated skb to be encrypted may be quoted if its source
> +	 * address (target of the error) is local.
> +	 */
> +	if (unlikely(skb_is_decrypted(skb_in)) &&
> +	    (skb_in->skb_iif || !(rt->rt_flags & RTCF_LOCAL)))
> +		goto ende;

[Severity: High]
Does this check cover every reject path the commit message names? The
commit message says:

    a reject rule (iptables REJECT, nft reject) sends its icmp error with
    icmp_send(), through nf_send_unreach(). so now, a received packet which
    the device decrypted, and which hits a reject rule in the INPUT or
    FORWARD chain, is now dropped without a reply, like a drop rule.

That is true for iptables REJECT and for the nft ip and inet families.
The nft netdev and bridge families are different. They build their own
ICMP error and never reach __icmp_send():

nft_reject_netdev_eval()
  nft_reject_netdev_send_v4_unreach()
    nf_reject_skb_v4_unreach()
    nft_reject_queue_xmit()
      dev_queue_xmit()

nft_reject_br_send_v4_unreach()
  nf_reject_skb_v4_unreach()
  br_forward()

nf_reject_skb_v4_unreach() copies the original packet into the error
and has no skb_is_decrypted() check:

net/ipv4/netfilter/nf_reject_ipv4.c:nf_reject_skb_v4_unreach() {
    ...
	/* RFC says return as much as we can without exceeding 576 bytes. */
	len = min_t(unsigned int, 536, oldskb->len);
    ...
	skb_put_data(nskb, skb_network_header(oldskb), len);
    ...
}

Take an nft netdev reject rule on the ingress hook of a NIC that does
kTLS or PSP RX offload. The ingress hook in __netif_receive_skb_core()
sees the skb after the driver has decrypted it.

Wouldn't the ICMP error then send the decrypted TCP payload (kTLS), or
the TCP header plus payload (PSP), out in cleartext?

The new nskb has decrypted == 0, so nothing later on the tx path stops
it. The bridge family uses the same helper. A decrypted skb on a bridge
port is less likely, but the code doesn't rule it out.

The rest of the series doesn't seem to close this gap either. The ipv6
follow-up, "ipv6: icmpv6: don't quote decrypted payload in icmpv6
errors", seems to leave the same gap in nf_reject_skb_v6_unreach() for
the netdev and bridge families.

Should the check also go into nf_reject_skb_v4_unreach() and
nf_reject_skb_v6_unreach(), or into their callers? If not, could the
commit message be narrowed to the reject paths that are actually
covered?

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