Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · 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).
- [Medium] In net/ipv4/icmp.c:__icmp_send(), the PSP-vs-kTLS check fails
open.
- [Low] Concern: icmp6_send() in net/ipv6/icmp.c has no
skb_is_decrypted()/SKB_EXT_PSP handling, so ICMPv6 errors would still…
--
ipv4: icmp: don't quote decrypted payload in icmp errors
This patch changes __icmp_send() so it no longer quotes device-decrypted
data in ICMP errors. For kTLS rx offload skbs, the quote is cut off at the
end of the TCP header and the RFC 4884 extension is skipped. For received
skbs with SKB_EXT_PSP, no error is sent at all.
quoted hunk ↗ jump to hunk
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 90c0e22c29bea..d522488475b61 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -979,6 +981,32 @@ 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 the decrypted part of an skb onto the network */
+ if (unlikely(skb_is_decrypted(skb_in))) {
[Severity: Low]
This only covers IPv4. In this patch, icmp6_send() in net/ipv6/icmp.c
still has no skb_is_decrypted() or SKB_EXT_PSP check. An ICMPv6 error
about a kTLS or PSP decrypted skb would still quote the decrypted payload
and the RFC 4884 extension.
The next patch in the series, "ipv6: icmpv6: don't quote decrypted
payload in icmpv6 errors", seems to add the same handling to icmp6_send().
So the series as a whole closes this gap.
If this patch is backported to stable without the IPv6 one, ICMPv6 stays
uncovered. Should the two be kept together for backports, or should this
commit mention the IPv6 follow-up?
quoted hunk ↗ jump to hunk
+ if (!skb_in->skb_iif) {
+ /* A locally originated skb to be encrypted. Permit
+ * the packet to be quoted, if its source address is
+ * local. otherwise skip.
+ */
+ if (!(rt->rt_flags & RTCF_LOCAL))
+ goto ende;
+ } else if (psp_skb_is_decrypted(skb_in)) {
+ /* PSP encrypts the transport header too, so it should
+ * not be quoted. But an error whose quote is truncated
+ * to fewer than 8 bytes of the transport header won't
+ * be processed, so just skip it.
+ */
+ goto ende;
+ } else {
+ /* TLS offload leaves the tcp header in the clear and
+ * so can be quoted up to its end. A psp skb whose inner
+ * tunnel was decapsulated also ends up here, since
+ * skb_scrub_packet() drops SKB_EXT_PSP.
+ */
+ decrypted = true;
+ }
+ }
[Severity: Medium]
Can PSP traffic fall through to the kTLS branch in more cases than the
tunnel decap case named in the comment?
psp_skb_is_decrypted() only checks for SKB_EXT_PSP. skb_scrub_packet()
resets the extensions but leaves skb->decrypted set:
net/core/skbuff.c:skb_scrub_packet() {
skb->pkt_type = PACKET_HOST;
skb->skb_iif = 0;
skb->ignore_df = 0;
skb_dst_drop(skb);
skb_ext_reset(skb);
...
}
bpf_redirect_peer() at tc ingress goes through skb_scrub_packet() in
skb_do_redirect():
net/core/filter.c:skb_do_redirect() {
...
if (flags & BPF_F_PEER) {
...
skb_scrub_packet(skb, false);
...
skb->dev = dev;
dev_sw_netstats_rx_add(dev, skb->len);
return -EAGAIN;
}
...
}
Receive then runs again in the peer netns, so skb_iif is non-zero again.
An ingress bpf_redirect() looks similar:
__bpf_rx_skb()->____dev_forward_skb()->skb_scrub_packet()
On those paths, a PSP decrypted skb reaches __icmp_send() with skb_iif
set but without SKB_EXT_PSP. It takes the decrypted = true branch. The
quote then holds the IP header plus max(th->doff * 4, 8) bytes of L4
header.
That is up to 60 bytes of TCP header (ports, seq/ack, window, options)
that PSP encrypted on the wire, sent back in a cleartext ICMP error. For
a non-TCP inner protocol it is 8 bytes of L4 header.
A netfilter REJECT, TTL exceeded or frag needed in the target netns could
trigger this. Host routed or bridged forwarding doesn't seem affected,
because sk_validate_xmit_skb() drops decrypted skbs that have no socket.
The BPF redirect and tunnel decap paths skip that check.
The commit message says "don't send the error at all for psp", but these
paths still quote the encrypted transport header. Would it be safer to
fail closed, for example by quoting only when the skb is known to be
kTLS, instead of whenever SKB_EXT_PSP is missing?
The IPv6 patch later in the series appears to have the same fallback.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103001143.246081-1-toddfreed%40meta.com