[PATCH net v5 0/8] psp, tls: don't send offloaded contents in cleartext

[PATCH net v5 0/8] psp, tls: don't send offloaded contents in cleartext

[Todd Freed]

While working on adding support for socket-less psp associations, I
noticed the case of ipv6 extension headers was not being handled
properly for psp packets. It will be relevant with that series, because
it adds udp support, for which the psp nexthdr needs to look at the
inner packet.

That led me to various other ways where packets on the crypto offload
path could be transmitted unencrypted.

Since the psp/tls association encodes intent to send encrypted packets,
but these various cases circumvent that, sending these as fixes.

The cases are,

 1/ ipv6 extension headers (patch 2; patch 1 makes mlx5e flush its tx
    queue when the resulting drop happens),
 2/ fragments (patch 3),
 3/ icmp errors (patch 4),
 4/ icmpv6 errors (patch 5),

Cases 2, 3 and 4 are relevant for ktls as well as psp.

patch 8 fixes an mlx5e bug in tso checksum calculation for psp packets
when the l3 dest is an intermediate hop (ipv4 source routing). It's not
about preventing unencrypted tx, so could be considered separate.

netfilter refragments bridged packets and builds reject replies itself,
outside these paths. A separate series to nf-next, which follows this
one, applies the same rules there.

Testing:

all hardware testing was done on connectx-7 (fw 28.48), between two x86
hosts, plus netdevsim.

 - drivers/net psp.py: on netdevsim, all 36 tests pass. on mlx5, 30
   pass, 4 skip (gmac versions the device doesn't support), and 2 xfail
   (netdevsim only).

 - tso checksum seed: a psp tcp socket with an ipv4 loose source route,
   the peer checking tcp checksums in software. hundreds of checksum
   errors before the fix, none after.

 - fragments, with tls offload and psp:
   - ipv4 (route mtu above the device mtu, IP_PMTUDISC_OMIT): without
     the fix, some fragments after the first go out in cleartext. with
     it they're dropped.
   - ipv6, through an ipvs dr service with a local client: without the
     fix, fragments after the first go out in cleartext, and the peer
     reassembles them. with it they're dropped.

 - icmp and icmpv6 errors, on netdevsim psp, with an nft reject rule
   (icmpx port-unreachable):
   - in an input chain, of received decrypted packets: without the fix,
     the errors go out on the wire, quoting the decrypted packets. with
     it, no error reaches the wire.
   - in an output chain, of a psp socket's packets: the error is
     delivered to the local socket, with and without the fix.
   - a PKT_TOOBIG from ip6_fragment() about a psp socket's packets is
     delivered to the socket, with and without the fix.

v5:
 - new patch 1: mlx5e rings the tx doorbell when an accel offload drops
   a packet, and checks the stop room after the nop it posts (Sashiko)
 - ipv6 extension headers: drop them in psp_dev_encapsulate() again,
   instead of in mlx5e. it's the helper which can't encapsulate them,
   for every driver (netdevsim sent them malformed). unlike the routing
   header rejection in v2, this isn't a device limitation
 - don't fragment decrypted skbs in ip6_fragment() either
 - icmp and icmpv6: send no error at all about a received decrypted skb,
   instead of quoting up to the tcp header for tls offload.
 - icmpv6: decide whether an error about a locally originated skb goes
   to a local address from the error's route (RTF_LOCAL), like icmp
   (RTCF_LOCAL), instead of the address list (Sashiko)
 - selftests: the extension header test runs on netdevsim too, and
   removes the socket option before closing. the fragment test uses the
   source address of the route, and turns off tcp_mtu_probing

v4: https://lore.kernel.org/netdev/20261103001143.246081-1-toddfreed@meta.com
 - back to dropping ipv6 packets with extension headers, but moved into
   mlx5e instead of psp_dev_encapsulate()
 - drop the psp core changes to rx and tx ipv6 extension headers
 - new patch 2 to drop decrypted skbs instead of fragmenting
 - new patches 3 and 4 to prevent quoting just-decrypted packet contents
   in icmp errors, v4 and v6 respectively (Jakub, Bjorn, Sashiko)
 - new selftest: ipv4 fragments of a psp tcp socket

v3: https://lore.kernel.org/netdev/20261023234640.1459291-1-toddfreed@meta.com
 - new patch 1 to fix tso checksum seed calculation in mlx5e (Daniel)
 - accept routing headers and reject ah (Daniel)
 - split the psp_dev_rcv() change into its own patch (Daniel)
 - return -EINVAL instead of -1 (Daniel)
 - selftest: one variant per extension header type (Daniel)

v2: https://lore.kernel.org/netdev/20261022223014.556388-1-toddfreed@meta.com
 - walk the extension headers instead of dropping (Daniel)
 - reject fragment headers (Bjorn, Daniel)
 - reject routing headers
 - psp_dev_rcv() handle the rx case as well

v1: https://lore.kernel.org/netdev/20261021210200.788191-1-toddfreed@meta.com

Todd Freed (8):
  net/mlx5e: ring the tx doorbell when an accel offload drops a packet
  psp: don't encapsulate ipv6 packets with extension headers
  net: don't fragment decrypted skbs
  ipv4: icmp: don't quote decrypted payload in icmp errors
  ipv6: icmpv6: don't quote decrypted payload in icmpv6 errors
  selftests: drv-net: psp: test ipv6 tcp socket with extension headers
  selftests: drv-net: psp: test that ipv4 psp packets aren't fragmented
  net/mlx5e: psp: tso: replace only the length in checksum seed

 .../mellanox/mlx5/core/en_accel/psp_rxtx.c    |  21 +-
 .../net/ethernet/mellanox/mlx5/core/en_tx.c   |   8 +-
 net/ipv4/icmp.c                               |  12 +
 net/ipv4/ip_output.c                          |   8 +
 net/ipv6/icmp.c                               |  16 ++
 net/ipv6/ip6_output.c                         |   9 +
 net/psp/psp_main.c                            |  21 +-
 tools/testing/selftests/drivers/net/config    |   1 +
 tools/testing/selftests/drivers/net/psp.py    | 249 +++++++++++++++++-
 9 files changed, 327 insertions(+), 18 deletions(-)


base-commit: 8012eec76b7e6c0d9fb338fe8e3db6ca1b419da4
-- 
2.53.0-Meta

[PATCH net v5 1/8] net/mlx5e: ring the tx doorbell when an accel offload drops a packet

[Todd Freed]

when mlx5e_accel_tx_begin() drops the skb, e.g. because psp can't
encapsulate it, mlx5e_xmit() returns without posting anything. the skbs
before it in a bulk dequeue may have been posted with xmit_more, which
defers the doorbell to the next one, and an mpwqe session may be open.
the device then doesn't see them until the next transmit on that sq, and
since the queue isn't stopped, the tx watchdog doesn't fire.

flush the sq on this drop like the dma error paths do: close the mpwqe
session, post a nop and ring the doorbell.

mlx5e_tx_flush() doesn't check whether the nop leaves less than
stop_room in the sq. for the dma error paths that is rare, but a run of
offload drops would post nops without stopping the queue, and could
overrun wqes in flight. so check it in mlx5e_tx_flush(), like the other
producers do. the completion of the nop wakes the queue again.

the next patch makes the psp drop reachable from user space, with an
ipv6 extension header set on a psp socket.

Fixes: 2ac9cfe78223 ("net/mlx5e: IPSec, Add Innova IPSec offload TX data path")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index 14479da98f42..006bf976def1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -365,6 +365,7 @@ static void mlx5e_tx_flush(struct mlx5e_txqsq *sq)
 	};
 
 	wqe = mlx5e_post_nop(&sq->wq, sq->sqn, &sq->pc);
+	mlx5e_tx_check_stop(sq);
 	mlx5e_notify_hw(&sq->wq, sq->pc, sq->uar_map, &wqe->ctrl);
 }
 
@@ -672,8 +673,13 @@ netdev_tx_t mlx5e_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	/* May send SKBs and WQEs. */
-	if (unlikely(!mlx5e_accel_tx_begin(dev, sq, skb, &accel)))
+	if (unlikely(!mlx5e_accel_tx_begin(dev, sq, skb, &accel))) {
+		/* The skb was dropped: ring the doorbell which the previous
+		 * ones may have deferred to it
+		 */
+		mlx5e_tx_flush(sq);
 		return NETDEV_TX_OK;
+	}
 
 	mlx5e_sq_xmit_prepare(sq, skb, &accel, &attr);
 
-- 
2.53.0-Meta

[PATCH net v5 2/8] psp: don't encapsulate ipv6 packets with extension headers

[Todd Freed]

psp_dev_encapsulate() inserts the udp+psp headers at the transport
header, rewrites the protocol of the fixed ip header to udp, and writes
tcp as the psp next header. so tcp must directly follow the fixed
header.

if the packet is an ipv6 packet with extension headers, udp+psp is
placed after them, but the next header indicating the l4 protocol is on
the last extension header in the chain, not in the fixed header.
therefore this produces a malformed packet.

in addition to being malformed, with mlx5 this kind of packet goes out
unencrypted. since the tx association encodes intent to send encrypted
packets, drop such packets instead.

a psp tcp socket emits such packets if certain socket options are set on
it (e.g. IPV6_DSTOPTS). refusing the combination of such socket options
with psp association would be helpful but is not implemented here. since
extension headers can also come from a route the datapath check is
needed anyway.

the check also covers ipv4: the l4 protocol must be tcp, and the
transport header must follow the ip header and its options. packets of
a psp tcp socket always pass it.

Fixes: fc724515741a ("psp: provide encapsulation helper for drivers")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 net/psp/psp_main.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
index 273b010d2355..c071f507261b 100644
--- a/net/psp/psp_main.c
+++ b/net/psp/psp_main.c
@@ -245,6 +245,24 @@ static void psp_write_headers(struct net *net, struct sk_buff *skb, __be32 spi,
 	memset(&psph->iv, 0, sizeof(psph->iv));
 }
 
+/* UDP+PSP goes in at the transport header, the protocol of the fixed IP
+ * header becomes UDP, and TCP the PSP next header: TCP must directly follow
+ * the fixed header. It doesn't with IPv6 extension headers, whose last one
+ * holds the L4 protocol.
+ */
+static bool psp_dev_encap_ok(const struct sk_buff *skb)
+{
+	u32 network_len = skb_network_header_len(skb);
+
+	if (skb->protocol == htons(ETH_P_IP))
+		return ip_hdr(skb)->protocol == IPPROTO_TCP &&
+		       network_len == ip_hdrlen(skb);
+	if (skb->protocol == htons(ETH_P_IPV6))
+		return ipv6_hdr(skb)->nexthdr == IPPROTO_TCP &&
+		       network_len == sizeof(struct ipv6hdr);
+	return false;
+}
+
 /* Encapsulate a TCP packet with PSP by adding the UDP+PSP headers and filling
  * them in.
  */
@@ -255,8 +273,7 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
 	u32 ethr_len = skb_mac_header_len(skb);
 	u32 bufflen = ethr_len + network_len;
 
-	if (skb->protocol != htons(ETH_P_IP) &&
-	    skb->protocol != htons(ETH_P_IPV6))
+	if (!psp_dev_encap_ok(skb))
 		return false;
 
 	if (skb_cow_head(skb, PSP_ENCAP_HLEN))
-- 
2.53.0-Meta

[PATCH net v5 3/8] net: don't fragment decrypted skbs

[Todd Freed]

on transmit, skb->decrypted means the device must encrypt the skb.
if it can't do that, then the packet should be dropped, not transmitted
in the clear.

ip_do_fragment() and ip6_fragment() on a decrypted skb produce
fragments which have skb->decrypted unset, and can be sent out
unencrypted.

tx of individually encrypted fragments would not work anyway, because
encrypting needs the L4 header, which non-initial fragments do not have.

ktls could encrypt in sw before fragmenting. that would not work for psp
offload which does not have a software fallback path.

tcp sizes segments to the mtu, so psp and ktls socket packets normally
won't reach this path. an ipv4 tcp socket can be made to send fragments
by setting a route mtu above the device mtu and disabling pmtud on the
socket (IP_PMTUDISC_OMIT). the selftest later in the series does this.

emitting fragments from an ipv6 tcp socket requires more steps, but is
also possible. the test setup here uses ipvs, which sets ignore_df on
forwarded packets. in ip6_fragment(), check skb->decrypted after the
too-big checks, so that a local socket still gets PKT_TOOBIG for path
mtu discovery.

tested with tls tx offload on mlx5 (connectx-7), over ipv4. without
this patch some fragments after the first go out in cleartext, with it
they're dropped (IpFragFails) and the connection stalls.

tested over ipv6, with tls tx offload and psp on mlx5 (connectx-7),
through an ipvs dr service with a local client. without this patch,
fragments after the first go out in cleartext, and the peer reassembles
them; with it they're dropped (Ip6FragFails).

Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
Fixes: 659a2899a57d ("tcp: add datapath logic for PSP with inline key exchange")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 net/ipv4/ip_output.c  | 8 ++++++++
 net/ipv6/ip6_output.c | 9 +++++++++
 2 files changed, 17 insertions(+)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index d42d776af273..94a40e7ed534 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -770,6 +770,14 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 	struct ip_frag_state state;
 	int err = 0;
 
+	/* PSP and TLS offload need the packet's L4 header, which non-initial
+	 * fragments do not have.
+	 */
+	if (unlikely(skb_is_decrypted(skb))) {
+		err = -EMSGSIZE;
+		goto fail;
+	}
+
 	/* for offloaded checksums cleanup checksum before fragmentation */
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		err = ip_check_csum_start(skb);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 25fe0ba98b1a..5f19d170ed29 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -929,6 +929,15 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 			mtu = IPV6_MIN_MTU;
 	}
 
+	/* PSP and TLS offload need the packet's L4 header, which non-initial
+	 * fragments do not have. Checked after the too-big checks above,
+	 * so that PMTUD for a local socket works.
+	 */
+	if (unlikely(skb_is_decrypted(skb))) {
+		err = -EMSGSIZE;
+		goto fail;
+	}
+
 	if (np) {
 		u32 frag_size = READ_ONCE(np->frag_size);
 
-- 
2.53.0-Meta

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

[Todd Freed]

an icmp error quotes as much of the invoking packet as fits in 576
bytes. if the device had decrypted the packet (as indicated by
skb->decrypted) then the icmp error is sending part of it back out onto
the network in cleartext.

the two offloads encrypt different parts of the packet:

- ktls encrypts the tcp payload. the tcp header is in the clear on the
  wire, so a quote of the ip header and the tcp header, without the
  payload, would be fine.

- psp encryption also covers the tcp header. the quote could include
  just the ip header, but then that's not really useful. linux receivers
  want to look at the l4 layer on the quoted packet to look up the
  related flow, so such a truncated icmp error packet is just dropped
  (icmp_socket_deliver()).

the two offloads can't be told apart reliably. a decrypted psp skb has
SKB_EXT_PSP, but some paths drop it and keep skb->decrypted
(skb_scrub_packet(), e.g. on bpf redirects and tunnel decap). so don't
send an error about a received decrypted skb at all.

on tx, skb->decrypted is also set. an error about a locally originated
skb (skb_iif is 0) is sent in full as before, if it is routed to a local
address (RTCF_LOCAL on the route to the packet's source address).

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.

packets which weren't decrypted are rejected as before, and so are
locally originated ones in the OUTPUT chain, if the error goes to a
local address as above.

the next patch does the same for icmpv6.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023234640.1459291-1-toddfreed%40meta.com
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Suggested-by: Björn Töpel <bjorn@kernel.org>
Fixes: 784abe24c903 ("net: Add decrypted field to skb")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 net/ipv4/icmp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 90c0e22c29be..e11008aa4cec 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;
+
 	/* peer icmp_ratelimit */
 	if (!icmpv4_xrlim_allow(net, rt, &fl4, type, code, apply_ratelimit))
 		goto ende;
-- 
2.53.0-Meta

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

[Todd Freed]

an icmpv6 error quotes as much of the invoking packet as fits in the
minimum ipv6 mtu. if the device had decrypted the packet (as indicated
by skb->decrypted) then the icmpv6 error is sending part of it back out
onto the network in cleartext.

as in the previous patch for icmp: ktls rx device offload and psp can't
be told apart reliably, and psp also encrypts the l4 header, so don't
send an error about a received decrypted skb at all. linux receivers
would drop an error whose quote stops before the l4 header anyway
(icmpv6_notify()).

on tx, skb->decrypted is also set. an error about a locally originated
skb (skb_iif is 0) is sent in full as before, if its route goes to a
local address (RTF_LOCAL), like RTCF_LOCAL in the previous patch.

an error about a received decrypted skb is dropped before the rate
limit, so that it doesn't use up the peer's and the global icmpv6
credit.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023234640.1459291-1-toddfreed%40meta.com
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Suggested-by: Björn Töpel <bjorn@kernel.org>
Fixes: 784abe24c903 ("net: Add decrypted field to skb")
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 net/ipv6/icmp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index d8203ebad882..949025d83e15 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;
+
 	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;
+	}
+
 	ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst);
 
 	msg.skb = skb;
-- 
2.53.0-Meta

[PATCH net v5 6/8] selftests: drv-net: psp: test ipv6 tcp socket with extension headers

[Todd Freed]

send on a psp tcp socket in three phases, checking the device's
tx-packets and what the peer received after each:

 1/ no socket options. the data is sent, counted, and received.

 2/ an extension header set on the socket. the packets are dropped,
    tx-packets is unchanged, and nothing reaches the peer. the data
    stays queued.

 3/ option removed. the queued data, then new data, is sent, counted,
    and received.

the test has a variant for each extension header a socket can set: a
destination options header (IPV6_DSTOPTS) and a hop-by-hop options
header (IPV6_HOPOPTS), each holding one padn option, and a segment
routing header (IPV6_RTHDR).

a software filter on the peer also counts, and drops, udp packets from
the socket's address to the peer's. psp packets reach the peer as
decrypted tcp packets, as expected. packets which were encapsulated but
are malformed or unencrypted (this is what mlx5 does) show up as udp,
and the test asserts there are none. if the filter cannot be set up the
test is skipped.

tx-packets is read after it has quiesced, because some devices refresh
counters periodically (mlx5 takes about a second).

without the fix in "psp: don't encapsulate ipv6 packets with extension
headers", netdevsim encapsulates the packets in a malformed way and
counts them in tx-packets, and the test fails on that. with mlx5
(connectx-7), the packets go out unencrypted and aren't counted in
tx-packets. the test fails on the udp packets which reach the peer.
capturing on the peer,

  tcpdump -i <dev> -nn -A 'ip6 and src host <sender> and not tcp'

shows them as udp, with the test's payload readable.

Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 tools/testing/selftests/drivers/net/config |   1 +
 tools/testing/selftests/drivers/net/psp.py | 148 ++++++++++++++++++++-
 2 files changed, 146 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/config b/tools/testing/selftests/drivers/net/config
index 4838adf27fa1..409f9a96a102 100644
--- a/tools/testing/selftests/drivers/net/config
+++ b/tools/testing/selftests/drivers/net/config
@@ -4,6 +4,7 @@ CONFIG_DEBUG_INFO_BTF_MODULES=n
 CONFIG_INET_PSP=y
 CONFIG_IPV6=y
 CONFIG_MACSEC=m
+CONFIG_NET_ACT_GACT=m
 CONFIG_NET_ACT_SKBEDIT=m
 CONFIG_NET_CLS_ACT=y
 CONFIG_NET_CLS_BPF=y
diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
index 473500901879..7e251439a2fd 100755
--- a/tools/testing/selftests/drivers/net/psp.py
+++ b/tools/testing/selftests/drivers/net/psp.py
@@ -18,12 +18,12 @@ from lib.py import ksft_run, ksft_exit, ksft_pr
 from lib.py import ksft_true, ksft_eq, ksft_ne, ksft_gt, ksft_raises
 from lib.py import ksft_not_none
 from lib.py import ksft_variants, KsftNamedVariant
-from lib.py import KsftSkipEx, KsftFailEx
+from lib.py import KsftSkipEx, KsftFailEx, CmdExitFailure
 from lib.py import NetDrvEpEnv, NetDrvContEnv
 from lib.py import Netlink, NlError, PSPFamily, RtnlFamily
 from lib.py import NetNSEnter
 from lib.py import bkg, rand_port, wait_port_listen
-from lib.py import ip
+from lib.py import ip, tc
 
 TCP_ULP = 31
 
@@ -133,6 +133,58 @@ def _check_data_outq(s, exp_len, force_wait=False):
 def _get_stat(cfg, key):
     return cfg.pspnl.get_stats({'dev-id': cfg.psp_dev_id})[key]
 
+
+def _get_quiesced_stat(cfg, key, timeout=10):
+    """
+    Like _get_stat(), but wait till the value stops changing. Note
+    that some devices refresh flow counters periodically, e.g. once
+    per second with mlx5.
+    """
+    val = _get_stat(cfg, key)
+    end = time.monotonic() + timeout
+    while True:
+        time.sleep(1.5)
+        new = _get_stat(cfg, key)
+        if new == val:
+            return val
+        val = new
+        if time.monotonic() >= end:
+            ksft_pr(f"{key} still changing after {timeout}s: {val}")
+            return val
+
+
+def _remote_count(cfg, proto, match):
+    """
+    Count, and drop, the @proto packets matching flower @match which reach
+    the remote, with a software filter on its ingress (offloaded filter
+    counters are refreshed periodically). Dropping keeps such packets away
+    from the remote's stack. Returns a function reading the count, or None
+    if the filter can't be installed.
+    """
+    flt = (f"dev {cfg.remote_ifname} ingress protocol {proto} pref 1 "
+           f"handle 0x5053 flower")
+    try:
+        qdiscs = tc(f"qdisc show dev {cfg.remote_ifname}", json=True,
+                    host=cfg.remote)
+        # An ingress qdisc takes the filter as well as clsact
+        if not any(q['kind'] in ('clsact', 'ingress') for q in qdiscs):
+            tc(f"qdisc add dev {cfg.remote_ifname} clsact", host=cfg.remote)
+            defer(tc, f"qdisc del dev {cfg.remote_ifname} clsact",
+                  host=cfg.remote)
+        tc(f"filter add {flt} skip_hw {match} action drop", host=cfg.remote)
+    except CmdExitFailure as e:
+        ksft_pr(f"Can't install a filter on the remote: {e}")
+        return None
+    defer(tc, f"filter del {flt}", host=cfg.remote)
+
+    def count():
+        filters = tc(f"-s filter get {flt}", json=True, host=cfg.remote)
+        for f in filters:
+            for act in f.get("options", {}).get("actions", []):
+                return act["stats"]["packets"]
+        raise KsftFailEx("no action on the remote filter")
+    return count
+
 #
 # Test case boiler plate
 #
@@ -658,6 +710,95 @@ def removal_device_bi(cfg):
         _close_conn(cfg, s)
 
 
+def _ipv6_exthdr_variants():
+    padn = bytes([0, 0, 1, 4, 0, 0, 0, 0])  # one PadN option
+
+    def srh(peer):
+        """ segment routing header, with @peer as its only segment """
+        return (bytes([0, 2, 4, 0, 0, 0, 0, 0]) +
+                socket.inet_pton(socket.AF_INET6, peer))
+
+    yield KsftNamedVariant("dstopts", socket.IPV6_DSTOPTS, lambda peer: padn)
+    yield KsftNamedVariant("hopopts", socket.IPV6_HOPOPTS, lambda peer: padn)
+    yield KsftNamedVariant("rthdr", socket.IPV6_RTHDR, srh)
+
+
+def _src_addr(dst):
+    """ The source address the kernel picks for @dst """
+    with socket.socket(socket.AF_INET6, socket.SOCK_DGRAM) as u:
+        u.connect((dst, 9))
+        return u.getsockname()[0]
+
+
+@ksft_variants(_ipv6_exthdr_variants())
+def data_send_ipv6_exthdr(cfg, opt, val):
+    """ Test that PSP drops IPv6 packets with an extension header """
+    cfg.require_ipver("6")
+    _init_psp_dev(cfg)
+
+    # Packets which were encapsulated but malformed or left unencrypted
+    # reach the remote as UDP. Set up the count before connecting, so a
+    # skip doesn't leave the responder mid key exchange.
+    peer = cfg.remote_addr_v["6"]
+    remote_udp = _remote_count(cfg, "ipv6", f"src_ip {_src_addr(peer)} "
+                               f"dst_ip {peer} ip_proto udp")
+    if not remote_udp:
+        raise KsftSkipEx("can't count UDP on the remote")
+
+    s = _make_psp_conn(cfg, 0, "6")
+    try:
+        rx_assoc = cfg.pspnl.rx_assoc({"version": 0,
+                                       "dev-id": cfg.psp_dev_id,
+                                       "sock-fd": s.fileno()})
+        tx = _spi_xchg(s, rx_assoc['rx-key'])
+        cfg.pspnl.tx_assoc({"dev-id": cfg.psp_dev_id,
+                            "version": 0,
+                            "tx-key": tx,
+                            "sock-fd": s.fileno()})
+        _check_data_outq(s, 0)
+
+        def wait_outq_empty():
+            for _ in range(100):
+                if _get_outq(s) == 0:
+                    break
+                time.sleep(0.5)
+            _check_data_outq(s, 0)
+
+        # 1. No option: sent, counted, and received
+        tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
+        total = _send_careful(cfg, s, 20)
+        wait_outq_empty()
+        ksft_gt(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts)
+        _check_data_rx(cfg, total)
+
+        # 2. With the option: dropped, not counted, and nothing reaches the
+        # peer. The data stays queued.
+        s.setsockopt(socket.IPPROTO_IPV6, opt, val(peer))
+        tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
+        udp = remote_udp()
+        queued = _send_careful(cfg, s, 20)
+        _check_data_outq(s, queued, force_wait=True)
+        ksft_eq(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts)
+        ksft_eq(remote_udp(), udp, comment="UDP at the remote")
+        _check_data_rx(cfg, total)
+        total += queued
+
+        # 3. Option removed: the queued data, then new data, sent and
+        # received
+        tx_pkts = _get_quiesced_stat(cfg, 'tx-packets')
+        s.setsockopt(socket.IPPROTO_IPV6, opt, b'')
+        wait_outq_empty()
+        total += _send_careful(cfg, s, 20)
+        wait_outq_empty()
+        ksft_gt(_get_quiesced_stat(cfg, 'tx-packets'), tx_pkts)
+        _check_data_rx(cfg, total)
+    finally:
+        # With the option, the queued data, the FIN and the ACKs would be
+        # dropped, leaving an orphan holding the association
+        s.setsockopt(socket.IPPROTO_IPV6, opt, b'')
+        _close_psp_conn(cfg, s)
+
+
 def _get_psp_ver_ip_variants():
     for ver in range(4):
         for ipv in ("4", "6"):
@@ -1173,7 +1314,8 @@ def main() -> None:
                                                           cfg.comm_port),
                                                          timeout=1)
 
-                cases = [data_basic_send, data_mss_adjust]
+                cases = [data_basic_send, data_mss_adjust,
+                         data_send_ipv6_exthdr]
 
                 if has_cont:
                     cases += [
-- 
2.53.0-Meta

[PATCH net v5 7/8] selftests: drv-net: psp: test that ipv4 psp packets aren't fragmented

[Todd Freed]

make the ipv4 packets of a psp tcp socket need fragmenting, and check
that they're dropped (IpFragFails) and not transmitted unencrypted.

tcp sizes its segments from the route mtu. with IP_PMTUDISC_OMIT, ip
fragments them to the device mtu, and doesn't set df. so the test sets a
route mtu of twice the larger of the two device mtus (at most 65535) on
both ends (the peer's mss limits ours too), keeping the device and
source address of the current route, and IP_PMTUDISC_OMIT on the
socket. tcp mtu probing (tcp_mtu_probing 2) would cap the mss at the
probing base, so the test turns it off. if the peer isn't on-link, or
the socket's mss doesn't need fragmenting, the test is skipped.

a software filter on the peer counts, and drops, the ipv4 fragments
which reach it from the socket's address, and the test asserts there are
none. IpFragFails is per netns, so it only confirms that the packets
were dropped.

while both route mtus are in place, a new ssh connection to the remote
over these addresses can stall: its segments are sized from the route
mtu, and sent with df set. so the local route is added last, and removed
before the next remote command.

no ipv6 test: the stack only fragments ipv6 tcp packets when something
like ipvs sets ignore_df on them (see "net: don't fragment decrypted
skbs"), which this test doesn't set up.

without the fix in "net: don't fragment decrypted skbs", the packets are
fragmented, and the fragments go out unencrypted. they reach the peer,
IpFragFails doesn't move, and the test fails.

Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 tools/testing/selftests/drivers/net/psp.py | 101 +++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
index 7e251439a2fd..5dac62ad96b0 100755
--- a/tools/testing/selftests/drivers/net/psp.py
+++ b/tools/testing/selftests/drivers/net/psp.py
@@ -26,6 +26,8 @@ from lib.py import bkg, rand_port, wait_port_listen
 from lib.py import ip, tc
 
 TCP_ULP = 31
+IP_MTU_DISCOVER = 10
+IP_PMTUDISC_OMIT = 5
 
 
 def _get_outq(s):
@@ -799,6 +801,105 @@ def data_send_ipv6_exthdr(cfg, opt, val):
         _close_psp_conn(cfg, s)
 
 
+def _ipv4_snmp(key):
+    """ An Ip: counter of this netns, from /proc/net/snmp """
+    with open("/proc/net/snmp", encoding="utf-8") as f:
+        ip_lines = [line.split() for line in f if line.startswith("Ip:")]
+    return int(ip_lines[1][ip_lines[0].index(key)])
+
+
+def _ipv4_route_mtu(dst, mtu, host=None):
+    """ Add a route to @dst with @mtu, keeping the device and source of
+    the current one, which must be on-link. Returns the defer removing it.
+    """
+    route = ip(f"-4 route get {dst}", json=True, host=host)[0]
+    if route.get("gateway"):
+        raise KsftSkipEx(f"{dst} isn't on-link")
+    try:
+        ip(f"route add {dst}/32 dev {route['dev']} src {route['prefsrc']} "
+           f"mtu {mtu}", host=host)
+    except CmdExitFailure as e:
+        raise KsftSkipEx(f"can't set the route mtu to {dst}: {e}") from e
+    return defer(ip, f"route del {dst}/32 dev {route['dev']}", host=host)
+
+
+def data_send_ipv4_frags(cfg):
+    """ Test that IPv4 packets of a PSP TCP socket aren't fragmented """
+    cfg.require_ipver("4")
+    _init_psp_dev(cfg)
+
+    # Fragments which get out reach the remote. Set up the count before
+    # connecting, so a skip doesn't leave the responder mid key exchange.
+    # The socket's source is the one the kernel picks, which may not be
+    # LOCAL_V4, and the fragments would leave through the route's device
+    rem = cfg.remote_addr_v["4"]
+    route = ip(f"-4 route get {rem}", json=True)[0]
+    if route["dev"] != cfg.ifname:
+        raise KsftSkipEx(f"{rem} isn't routed through {cfg.ifname}")
+    loc = route["prefsrc"]
+    remote_frags = _remote_count(cfg, "ip",
+                                 f"src_ip {loc} dst_ip {rem} ip_flags frag")
+    if not remote_frags:
+        raise KsftSkipEx("can't count fragments on the remote")
+
+    # TCP sizes its segments from the route MTU. With IP_PMTUDISC_OMIT, IP
+    # fragments them to the device MTU. Set a route MTU above the device
+    # MTU on both ends, since the peer's MSS limits ours too. While both
+    # are in place, a new ssh connection to the remote over these addresses
+    # can stall, so add the local one last, and remove it first.
+    mtu = min(2 * max(cfg.dev['mtu'], cfg.remote_dev['mtu']), 65535)
+    _ipv4_route_mtu(loc, mtu, host=cfg.remote)
+    local_route = _ipv4_route_mtu(rem, mtu)
+
+    # With MTU probing on, TCP caps the MSS at the probing base, whatever
+    # the route MTU
+    probing_path = "/proc/sys/net/ipv4/tcp_mtu_probing"
+    with open(probing_path, encoding="utf-8") as fp:
+        probing = fp.read().strip()
+    if int(probing) > 1:
+        def set_probing(val):
+            with open(probing_path, "w", encoding="utf-8") as fp:
+                fp.write(val)
+        set_probing("0")
+        defer(set_probing, probing)
+
+    s = _make_psp_conn(cfg, 0, "4")
+    try:
+        rx_assoc = cfg.pspnl.rx_assoc({"version": 0,
+                                       "dev-id": cfg.psp_dev_id,
+                                       "sock-fd": s.fileno()})
+        tx = _spi_xchg(s, rx_assoc['rx-key'])
+        cfg.pspnl.tx_assoc({"dev-id": cfg.psp_dev_id,
+                            "version": 0,
+                            "tx-key": tx,
+                            "sock-fd": s.fileno()})
+        s.setsockopt(socket.IPPROTO_IP, IP_MTU_DISCOVER, IP_PMTUDISC_OMIT)
+        mss = s.getsockopt(socket.IPPROTO_TCP, socket.TCP_MAXSEG)
+        if mss + 40 <= cfg.dev['mtu']:
+            raise KsftSkipEx(f"mss {mss} needs no fragmenting")
+
+        fails = _ipv4_snmp("FragFails")
+        # The packets are dropped, so the send may not complete (with a
+        # jumbo device MTU, the socket buffer can take all of it)
+        s.settimeout(3)
+        try:
+            s.sendall(b'f' * 200000)
+        except socket.timeout:
+            pass
+        fails = _ipv4_snmp("FragFails") - fails
+
+        # Before the next ssh to the remote, see above
+        local_route.exec()
+
+        # Fragments would go out unencrypted: the packets must be dropped
+        # instead of fragmented. FragFails is per netns, so it only
+        # confirms the drop: the remote count is what decides.
+        ksft_eq(remote_frags(), 0, comment="fragments reached the remote")
+        ksft_gt(fails, 0, comment="packets dropped instead of fragmented")
+    finally:
+        _close_psp_conn(cfg, s)
+
+
 def _get_psp_ver_ip_variants():
     for ver in range(4):
         for ipv in ("4", "6"):
-- 
2.53.0-Meta

[PATCH net v5 8/8] net/mlx5e: psp: tso: replace only the length in checksum seed

[Todd Freed]

for psp tcp with tso,

 - __tcp_transmit_skb() -> tcp_v6_send_check() or tcp_v4_send_check()
   computes the checksum seed using the addresses on the socket and the
   total length, then

 - mlx5e_psp_handle_tx_skb() recomputes the checksum seed using
   addresses from the packet's l3 header, and the length of one segment

the length difference is expected by the device. however, the difference
in addresses is incorrect in cases where the l3 header daddr is an
intermediate hop, for example,

 - ipv4 source route option (IP_OPTIONS with lsrr or ssrr)
 - ipv6 IPV6_RTHDR header (conceptually ; ipv6 extension headers are not
   properly supported on this path yet)

in these cases every segment of the tso packet to go out with a bad tcp
checksum

the fix is to replace only the length in the seed which tcp had already
computed. this is also what tcp_gso_segment() does.

tested on connectx-7, with a psp tcp socket whose ipv4 loose source
route's first hop isn't the final destination, and the peer checking
tcp checksums in software.

Fixes: e5a1861a298e ("net/mlx5e: Implement PSP Tx data path")
Acked-by: Daniel Zahka <daniel.zahka@gmail.com>
Signed-off-by: Todd Freed <toddfreed@meta.com>
---
 .../mellanox/mlx5/core/en_accel/psp_rxtx.c    | 21 ++++++++-----------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c
index 348fd7a96261..d9c2742b3c24 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.c
@@ -177,18 +177,15 @@ bool mlx5e_psp_handle_tx_skb(struct net_device *netdev,
 		return false;
 	}
 	if (skb_is_gso(skb)) {
-		int len = skb_shinfo(skb)->gso_size + inner_tcp_hdrlen(skb);
-		struct tcphdr *th = inner_tcp_hdr(skb);
-
-		if (skb->protocol == htons(ETH_P_IP)) {
-			const struct iphdr *ip = ip_hdr(skb);
-
-			th->check = ~tcp_v4_check(len, ip->saddr, ip->daddr, 0);
-		} else {
-			const struct ipv6hdr *ip6 = ipv6_hdr(skb);
-
-			th->check = ~tcp_v6_check(len, &ip6->saddr, &ip6->daddr, 0);
-		}
+		u32 seg_len = skb_shinfo(skb)->gso_size + inner_tcp_hdrlen(skb);
+		u32 len = skb->len - skb_inner_transport_offset(skb);
+
+		/* update just the length component of the checksum seed
+		 * tcp computed the seed with the total length, but mlx5
+		 * devices are expecting the segment length
+		 */
+		inet_proto_csum_replace4(&inner_tcp_hdr(skb)->check, skb,
+					 htonl(len), htonl(seg_len), true);
 	}
 
 	return true;
-- 
2.53.0-Meta

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

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

Re: [PATCH net v5 6/8] selftests: drv-net: psp: test ipv6 tcp socket with extension headers

[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] In data_send_ipv6_exthdr()
  (tools/testing/selftests/drivers/net/psp.py), the finally block runs…
- [Low] Concern: the commit message overstates coverage ('a variant for
  each extension header a socket can set') because there is no…

--

selftests: drv-net: psp: test ipv6 tcp socket with extension headers

This adds a PSP selftest that sends data on an IPv6 PSP TCP socket in
three phases: with no extension header, with one set (IPV6_DSTOPTS,
IPV6_HOPOPTS or IPV6_RTHDR), and after the option is removed. After each
phase it checks the device's tx-packets and a tc flower UDP counter on
the remote. It also adds a helper that waits for stats to quiesce, and
adds CONFIG_NET_ACT_GACT to the config.

> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
> index 473500901879b..7e251439a2fd9 100755
> --- a/tools/testing/selftests/drivers/net/psp.py
> +++ b/tools/testing/selftests/drivers/net/psp.py

[ ... ]

> @@ -658,6 +710,95 @@ def removal_device_bi(cfg):

[ ... ]

> +@ksft_variants(_ipv6_exthdr_variants())
> +def data_send_ipv6_exthdr(cfg, opt, val):
> +    """ Test that PSP drops IPv6 packets with an extension header """

[ ... ]

> +        # 2. With the option: dropped, not counted, and nothing reaches the
> +        # peer. The data stays queued.
> +        s.setsockopt(socket.IPPROTO_IPV6, opt, val(peer))

[ ... ]

> +    finally:
> +        # With the option, the queued data, the FIN and the ACKs would be
> +        # dropped, leaving an orphan holding the association
> +        s.setsockopt(socket.IPPROTO_IPV6, opt, b'')
> +        _close_psp_conn(cfg, s)

[Severity: Low]
What happens to this cleanup in data_send_ipv6_exthdr() when the test
runs without CAP_NET_RAW, for the dstopts and hopopts variants?

ipv6_set_opt_hdr() checks the capability before it reaches the
zero-length removal path. That means clearing the option with b'' can
also fail with EPERM:

net/ipv6/ipv6_sockglue.c:ipv6_set_opt_hdr() {
    ...
	/* hop-by-hop / destination options are privileged option */
	if (optname != IPV6_RTHDR && !sockopt_ns_capable(net->user_ns, CAP_NET_RAW))
		return -EPERM;

	/* remove any sticky options header with a zero option
	 * length, per RFC3542.
	 */
	if (optlen > 0) {
    ...
}

In that case the phase 2 setsockopt raises PermissionError first. Then
the setsockopt in the finally block raises a second PermissionError, so
_close_psp_conn() never runs.

When that happens, psp_responder never gets the "data close" command and
keeps its data socket open. The local PSP socket isn't closed explicitly
either, and the original exception ends up chained under the second one.
The following test cases share cfg.comm_sock and the responder state, so
they could be affected.

The rthdr variant isn't affected because IPV6_RTHDR skips the capability
check.

Could the option removal be guarded separately, for example with a
nested try/finally, so that _close_psp_conn() always runs?

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