[PATCH net v2] octeontx2-pf: fix skb use-after-free with ipsec crypto offload

[PATCH net v2] octeontx2-pf: fix skb use-after-free with ipsec crypto offload

[Nicolai Buchwitz]

otx2_dma_map_skb_frag() calls skb_unshare() on its own copy of the skb
pointer. For a cloned skb, for example with tcpdump running on the
interface, this frees the skb and the new copy is lost on return. The
transmit path then uses the freed skb and the copy leaks.

Make the skb writable in place in otx2_sq_append_skb() instead, so the
pointer stays valid. Also do this for skbs with shared frags, as the
hardware writes the encrypted data back into them.

Fixes: 195c3d463181 ("octeontx2-pf: map skb data as device writeable")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
This was flagged in the clashiko review of my skb_unshare() rename patch [1].

Compile-tested only, I have no octeontx2 hardware.

[1] https://lore.kernel.org/all/20261015-nb-skb-unclone-full-v1-1-0dfbf01f60f2@tipi-net.de/
---
Changes in v2:
- Also handle skbs with shared frags (Jakub)
- Note: conflicts with "net: rename skb_unshare() to skb_unclone_full()"
  in net-next, the resolution is to drop the skb_unclone_full() line
- Link to v1: https://patch.msgid.link/20261019-nb-otx2-xfrm-unshare-net-v1-1-ee9b24f9e673@tipi-net.de

To: Sunil Goutham <sgoutham@marvell.com>
To: Geetha sowjanya <gakula@marvell.com>
To: Ratheesh Kannoth <rkannoth@marvell.com>
To: Subbaraya Sundeep <sbhatta@marvell.com>
To: Bharat Bhushan <bbhushan2@marvell.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 4 +---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c   | 8 ++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index b11389a841d15..91ebf247f5f90 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -2121,10 +2121,8 @@ dma_addr_t otx2_dma_map_skb_frag(struct otx2_nic *pfvf,
 	int offset;
 
 	/* Crypto hardware need write permission for ipsec crypto offload */
-	if (unlikely(xfrm_offload(skb))) {
+	if (unlikely(xfrm_offload(skb)))
 		dir = DMA_BIDIRECTIONAL;
-		skb = skb_unshare(skb, GFP_ATOMIC);
-	}
 
 	/* First segment is always skb->data */
 	if (!seg) {
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index 8d2d607bc92f9..b98842c7e40e0 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -1205,6 +1205,14 @@ bool otx2_sq_append_skb(void *dev, struct netdev_queue *txq,
 	if (free_desc < otx2_get_sqe_count(pfvf, skb))
 		return false;
 
+	/* Crypto hardware need write permission for ipsec crypto offload */
+	if (unlikely(xfrm_offload(skb)) &&
+	    (skb_cloned(skb) || skb_has_shared_frag(skb)) &&
+	    skb_ensure_writable(skb, skb->len)) {
+		dev_kfree_skb_any(skb);
+		return true;
+	}
+
 	num_segs = skb_shinfo(skb)->nr_frags + 1;
 
 	/* If SKB doesn't fit in a single SQE, linearize it.

---
base-commit: 82e31923c0553fdbfcddcd910ca07952880ca9b9
change-id: 20261019-nb-otx2-xfrm-unshare-net-6b74a96a8d9e

Best regards,
--  
Nicolai Buchwitz <nb@tipi-net.de>

Re: [PATCH net v2] octeontx2-pf: fix skb use-after-free with ipsec crypto offload

[netdev-bot+sashiko]

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

Critical: 0 (1) · High: 1 (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] The fix only partly covers its target.
- [Low] When skb_ensure_writable() fails, the new drop path in
  otx2_sq_append_skb() (otx2_txrx.c:1211-1213) frees the skb with…

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but it is the
  same class of bug (the transmit path using a freed skb) in the code…
- [High] This problem wasn't introduced by this patch, but it comes from
  the commit being fixed (195c3d463181), and the patch neither fixes nor…

--

octeontx2-pf: fix skb use-after-free with ipsec crypto offload

This commit removes the skb_unshare() call from otx2_dma_map_skb_frag().
That call freed the caller's skb when it was cloned. The commit instead
calls skb_ensure_writable() in otx2_sq_append_skb() for xfrm offload skbs
that are cloned or have shared frags.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index b11389a841d1..91ebf247f5f9 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -2121,10 +2121,8 @@ dma_addr_t otx2_dma_map_skb_frag(struct otx2_nic *pfvf,
>  	int offset;
>  
>  	/* Crypto hardware need write permission for ipsec crypto offload */
> -	if (unlikely(xfrm_offload(skb))) {
> +	if (unlikely(xfrm_offload(skb)))
>  		dir = DMA_BIDIRECTIONAL;
> -		skb = skb_unshare(skb, GFP_ATOMIC);
> -	}

[Severity: High]
This is a pre-existing issue from the commit being fixed (195c3d463181).
It was not introduced by this patch.

The unmap side of this direction logic reads sg->skb without checking it.
In otx2_dma_unmap_skb_frags():

	skb = (struct sk_buff *)sg->skb;
	if (unlikely(xfrm_offload(skb)))
		dir = DMA_BIDIRECTIONAL;

On the DMA mapping error paths, sg->skb has not been set yet.
otx2_sqe_add_sg() returns before assigning it:

	dma_addr = otx2_dma_map_skb_frag(pfvf, skb, seg, &len);
	if (dma_mapping_error(pfvf->dev, dma_addr))
		return false;
	...
	sq->sg[sq->head].skb = (u64)skb;

After that, otx2_sq_append_skb() calls
otx2_dma_unmap_skb_frags(pfvf, &sq->sg[sq->head]).

otx2_sqe_add_sg_ipsec() returns the same way. The unmap label in
otx2_dma_map_tso_skb() works on sq->sg[first_sqe], whose skb is only
assigned after the last segment.

The completion paths reset sg->skb to NULL after each packet. On a
CONFIG_XFRM kernel, can a DMA mapping failure make
xfrm_offload(NULL)->skb_sec_path()->skb_ext_find() dereference NULL? This
applies to non-IPsec traffic too. After the cn10k_ipsec_transmit() drop
path, sg->skb could also be a stale pointer to a freed skb.

Would it work to pass the direction or the skb in explicitly, or to set
sg->skb before mapping?

>  
>  	/* First segment is always skb->data */
>  	if (!seg) {
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> index 8d2d607bc92f..b98842c7e40e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> @@ -1205,6 +1205,14 @@ bool otx2_sq_append_skb(void *dev, struct netdev_queue *txq,
>  	if (free_desc < otx2_get_sqe_count(pfvf, skb))
>  		return false;
>  
> +	/* Crypto hardware need write permission for ipsec crypto offload */
> +	if (unlikely(xfrm_offload(skb)) &&
> +	    (skb_cloned(skb) || skb_has_shared_frag(skb)) &&
> +	    skb_ensure_writable(skb, skb->len)) {

[Severity: High]
Does this condition catch every frag that the hardware will overwrite?
The commit message says:

  Also do this for skbs with shared frags, as the hardware writes the
  encrypted data back into them.

skb_has_shared_frag() only tests SKBFL_SHARED_FRAG. That flag is set for
splice, MSG_SPLICE_PAGES and zerocopy. It is not set when frag pages are
shared through ordinary page references.

cn10k_ipsec_init() advertises NETIF_F_HW_ESP but not NETIF_F_GSO_ESP:

	netdev->hw_features |= NETIF_F_HW_ESP;
	netdev->hw_enc_features |= NETIF_F_HW_ESP;

So TCP GSO over an offloaded SA is segmented in software:

validate_xmit_skb()
  esp4_gso_segment()
    tcp_gso_segment()
      skb_segment()

skb_segment() gives each segment a fresh, non-cloned head. It takes page
references on the same frag pages that the TCP retransmit queue skb still
holds. It only inherits the flag:

	skb_shinfo(nskb)->flags |= (skb_shinfo(head_skb)->flags |
				    skb_shinfo(frag_skb)->flags) &
				   SKBFL_SHARED_FRAG;

Plain tcp_sendmsg() data doesn't carry that flag. esp_output_head() then
takes its !skb_cloned() path and appends a trailer frag. The original
frags stay in place.

Such a segment fails both skb_cloned() and skb_has_shared_frag(), so
skb_ensure_writable() is skipped. otx2_sqe_add_sg_ipsec() then maps the
frag pages DMA_BIDIRECTIONAL, and cn10k_ipsec_transmit() does:

	inst.rptr = inst.dptr;

That means CPT writes ciphertext into pages TCP still considers
plaintext.

On a later retransmission, would TCP send that ciphertext as payload? The
TCP checksum is recomputed in software during skb_segment(), so the peer
would accept the corrupted data.

The core stack treats these frags as read-only. skb_cow_data() says
"pages are considered not writable at the moment even if they are
anonymous". Software ESP sets esp->inplace = false for nonlinear non-cloned
skbs.

The old skb_unshare() code had the same gap. Should the check be
skb_cloned(skb) || skb_is_nonlinear(skb) instead, or should the driver use
a separate output buffer, rather than relying on SKBFL_SHARED_FRAG?

> +		dev_kfree_skb_any(skb);
> +		return true;
> +	}

[Severity: Low]
When skb_ensure_writable() fails, the skb is freed and true is returned,
but no drop counter is incremented. otx2_xmit() counts its own drops:

	atomic_long_inc(&dev_stats->tx_discards);
	dev_kfree_skb(skb);
	return NETDEV_TX_OK;

Should this new path bump tx_discards too, so the drop shows up in
tx_dropped? The existing __skb_linearize() failure path in this function
has the same gap.

> +
>  	num_segs = skb_shinfo(skb)->nr_frags + 1;
>  
>  	/* If SKB doesn't fit in a single SQE, linearize it.

[Severity: Critical]
This is a pre-existing issue and was not introduced by this patch. Every
xfrm offload skb that passes the new check ends up in
cn10k_ipsec_transmit(), which can use a freed skb in a similar way:

cn10k_ipsec_transmit() {
	...
	sa_info = (struct qmem *)x->xso.offload_handle;
	if (!sa_info)
		goto drop;
	...
drop:
	dev_kfree_skb_any(skb);
	return false;
}

otx2_sq_append_skb() passes that false straight back, and otx2_xmit()
treats it as a full ring:

	if (!otx2_sq_append_skb(pf, txq, sq, skb, qidx)) {
		netif_tx_stop_queue(txq);
		...
		return NETDEV_TX_BUSY;
	}

Can the core then requeue and retransmit the skb that was already freed?
The VF and representor xmit handlers do the same.

This drop path looks reachable during SA deletion or rekey under traffic.
cn10k_ipsec_del_state() does:

	x->xso.offload_handle = 0;

Packets already queued with a sec_path reference to that state still
reach the driver. Clearing OTX2_FLAG_IPSEC_OFFLOAD_ENABLED in
cn10k_outb_cpt_clean() leads to the same label.

The DMA mappings that otx2_sqe_add_sg_ipsec() set up in sq->sg[sq->head]
are not unmapped on this path either. The next xmit resets num_segs for
that slot. Are those IOMMU mappings leaked?

Should the drop path unmap sq->sg[sq->head] and return true once the skb
is freed?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-nb-otx2-xfrm-unshare-net-v2-1-5590c9325bcd%40tipi-net.de