[PATCH 1/2 net V2] net: vertexcom: mse102x: Fix possible double free of TX skb

[PATCH 1/2 net V2] net: vertexcom: mse102x: Fix possible double free of TX skb

[Stefan Wahren]

The scope of the TX skb is wider than just mse102x_tx_frame_spi(),
so in case the TX skb room needs to be expanded, we should free the
the temporary skb instead of the original skb. Otherwise the original
TX skb pointer would be freed again in mse102x_tx_work(), which leads
to crashes:

  Internal error: Oops: 0000000096000004 [#2] PREEMPT SMP
  CPU: 0 PID: 712 Comm: kworker/0:1 Tainted: G      D            6.6.23
  Hardware name: chargebyte Charge SOM DC-ONE (DT)
  Workqueue: events mse102x_tx_work [mse102x]
  pstate: 20400009 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : skb_release_data+0xb8/0x1d8
  lr : skb_release_data+0x1ac/0x1d8
  sp : ffff8000819a3cc0
  x29: ffff8000819a3cc0 x28: ffff0000046daa60 x27: ffff0000057f2dc0
  x26: ffff000005386c00 x25: 0000000000000002 x24: 00000000ffffffff
  x23: 0000000000000000 x22: 0000000000000001 x21: ffff0000057f2e50
  x20: 0000000000000006 x19: 0000000000000000 x18: ffff00003fdacfcc
  x17: e69ad452d0c49def x16: 84a005feff870102 x15: 0000000000000000
  x14: 000000000000024a x13: 0000000000000002 x12: 0000000000000000
  x11: 0000000000000400 x10: 0000000000000930 x9 : ffff00003fd913e8
  x8 : fffffc00001bc008
  x7 : 0000000000000000 x6 : 0000000000000008
  x5 : ffff00003fd91340 x4 : 0000000000000000 x3 : 0000000000000009
  x2 : 00000000fffffffe x1 : 0000000000000000 x0 : 0000000000000000
  Call trace:
   skb_release_data+0xb8/0x1d8
   kfree_skb_reason+0x48/0xb0
   mse102x_tx_work+0x164/0x35c [mse102x]
   process_one_work+0x138/0x260
   worker_thread+0x32c/0x438
   kthread+0x118/0x11c
   ret_from_fork+0x10/0x20
  Code: aa1303e0 97fffab6 72001c1f 54000141 (f9400660)

Cc: stable@vger.kernel.org
Fixes: 2f207cbf0dd4 ("net: vertexcom: Add MSE102x SPI support")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/vertexcom/mse102x.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
index a04d4073def9..2c37957478fb 100644
--- a/drivers/net/ethernet/vertexcom/mse102x.c
+++ b/drivers/net/ethernet/vertexcom/mse102x.c
@@ -222,7 +222,7 @@ static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
 	struct mse102x_net_spi *mses = to_mse102x_spi(mse);
 	struct spi_transfer *xfer = &mses->spi_xfer;
 	struct spi_message *msg = &mses->spi_msg;
-	struct sk_buff *tskb;
+	struct sk_buff *tskb = NULL;
 	int ret;

 	netif_dbg(mse, tx_queued, mse->ndev, "%s: skb %p, %d@%p\n",
@@ -235,7 +235,6 @@ static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
 		if (!tskb)
 			return -ENOMEM;

-		dev_kfree_skb(txp);
 		txp = tskb;
 	}

@@ -257,6 +256,8 @@ static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
 		mse->stats.xfer_err++;
 	}

+	dev_kfree_skb(tskb);
+
 	return ret;
 }

--
2.34.1

[PATCH 0/2 net V2] net: vertexcom: mse102x: Fix mse102x_tx_work

[Stefan Wahren]

This fixes two issue in the TX path of the Vertexcom MSE102x driver.

Initial version:
https://lore.kernel.org/netdev/20241022155242.33729-1-wahrenst@gmx.net/

Changes in V2:
- free the temporary skb in patch 1 in order to minimize changes
  as suggested by Jakub Kicinski
- add patch 2 to also fix the tx_bytes calculation

Stefan Wahren (2):
  net: vertexcom: mse102x: Fix possible double free of TX skb
  net: vertexcom: mse102x: Fix tx_bytes calculation

 drivers/net/ethernet/vertexcom/mse102x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

--
2.34.1

[PATCH 2/2 net V2] net: vertexcom: mse102x: Fix tx_bytes calculation

[Stefan Wahren]

The tx_bytes should consider the actual size of the Ethernet frames
without the SPI encapsulation. But we still need to take care of
Ethernet padding.

Fixes: 2f207cbf0dd4 ("net: vertexcom: Add MSE102x SPI support")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/net/ethernet/vertexcom/mse102x.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
index 2c37957478fb..1fffae6596de 100644
--- a/drivers/net/ethernet/vertexcom/mse102x.c
+++ b/drivers/net/ethernet/vertexcom/mse102x.c
@@ -443,7 +443,8 @@ static void mse102x_tx_work(struct work_struct *work)
 		if (ret) {
 			mse->ndev->stats.tx_dropped++;
 		} else {
-			mse->ndev->stats.tx_bytes += txb->len;
+			mse->ndev->stats.tx_bytes += max_t(unsigned int,
+							   txb->len, ETH_ZLEN);
 			mse->ndev->stats.tx_packets++;
 		}

--
2.34.1

Re: [PATCH 2/2 net V2] net: vertexcom: mse102x: Fix tx_bytes calculation

[Jakub Kicinski]

On Tue,  5 Nov 2024 17:35:45 +0100 Stefan Wahren wrote:
> +			mse->ndev->stats.tx_bytes += max_t(unsigned int,
> +							   txb->len, ETH_ZLEN);

Is this enough? skb->len will include 2 bytes added by
mse102x_push_header() and 2 added by mse102x_put_footer()
(unless we went to skb_copy_expand(), which is very likely)

May be easier to save the skb->len before calling mse102x_tx_pkt_spi()