1. Patchew
  2. linux
  3. View series

[PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb

Stefan Wahren posted 1 patch 1 month ago 
drivers/net/ethernet/vertexcom/mse102x.c | 34 ++++++++++++------------
1 file changed, 17 insertions(+), 17 deletions(-)
[PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb
Posted by Stefan Wahren 1 month ago 
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, also its pointer
needs to be adjusted. Otherwise the already freed 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 | 34 ++++++++++++------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/vertexcom/mse102x.c b/drivers/net/ethernet/vertexcom/mse102x.c
index a04d4073def9..8b9e700a1e63 100644
--- a/drivers/net/ethernet/vertexcom/mse102x.c
+++ b/drivers/net/ethernet/vertexcom/mse102x.c
@@ -216,7 +216,7 @@ static inline void mse102x_put_footer(struct sk_buff *skb)
 	*footer = cpu_to_be16(DET_DFT);
 }

-static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
+static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff **txp,
 				unsigned int pad)
 {
 	struct mse102x_net_spi *mses = to_mse102x_spi(mse);
@@ -226,29 +226,29 @@ static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
 	int ret;

 	netif_dbg(mse, tx_queued, mse->ndev, "%s: skb %p, %d@%p\n",
-		  __func__, txp, txp->len, txp->data);
+		  __func__, *txp, (*txp)->len, (*txp)->data);

-	if ((skb_headroom(txp) < DET_SOF_LEN) ||
-	    (skb_tailroom(txp) < DET_DFT_LEN + pad)) {
-		tskb = skb_copy_expand(txp, DET_SOF_LEN, DET_DFT_LEN + pad,
+	if ((skb_headroom(*txp) < DET_SOF_LEN) ||
+	    (skb_tailroom(*txp) < DET_DFT_LEN + pad)) {
+		tskb = skb_copy_expand(*txp, DET_SOF_LEN, DET_DFT_LEN + pad,
 				       GFP_KERNEL);
 		if (!tskb)
 			return -ENOMEM;

-		dev_kfree_skb(txp);
-		txp = tskb;
+		dev_kfree_skb(*txp);
+		*txp = tskb;
 	}

-	mse102x_push_header(txp);
+	mse102x_push_header(*txp);

 	if (pad)
-		skb_put_zero(txp, pad);
+		skb_put_zero(*txp, pad);

-	mse102x_put_footer(txp);
+	mse102x_put_footer(*txp);

-	xfer->tx_buf = txp->data;
+	xfer->tx_buf = (*txp)->data;
 	xfer->rx_buf = NULL;
-	xfer->len = txp->len;
+	xfer->len = (*txp)->len;

 	ret = spi_sync(mses->spidev, msg);
 	if (ret < 0) {
@@ -368,7 +368,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
 	mse->ndev->stats.rx_bytes += rxlen;
 }

-static int mse102x_tx_pkt_spi(struct mse102x_net *mse, struct sk_buff *txb,
+static int mse102x_tx_pkt_spi(struct mse102x_net *mse, struct sk_buff **txb,
 			      unsigned long work_timeout)
 {
 	unsigned int pad = 0;
@@ -377,11 +377,11 @@ static int mse102x_tx_pkt_spi(struct mse102x_net *mse, struct sk_buff *txb,
 	int ret;
 	bool first = true;

-	if (txb->len < ETH_ZLEN)
-		pad = ETH_ZLEN - txb->len;
+	if ((*txb)->len < ETH_ZLEN)
+		pad = ETH_ZLEN - (*txb)->len;

 	while (1) {
-		mse102x_tx_cmd_spi(mse, CMD_RTS | (txb->len + pad));
+		mse102x_tx_cmd_spi(mse, CMD_RTS | ((*txb)->len + pad));
 		ret = mse102x_rx_cmd_spi(mse, (u8 *)&rx);
 		cmd_resp = be16_to_cpu(rx);

@@ -437,7 +437,7 @@ static void mse102x_tx_work(struct work_struct *work)

 	while ((txb = skb_dequeue(&mse->txq))) {
 		mutex_lock(&mses->lock);
-		ret = mse102x_tx_pkt_spi(mse, txb, work_timeout);
+		ret = mse102x_tx_pkt_spi(mse, &txb, work_timeout);
 		mutex_unlock(&mses->lock);
 		if (ret) {
 			mse->ndev->stats.tx_dropped++;
--
2.34.1
Re: [PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb
Posted by Jakub Kicinski 3 weeks, 6 days ago 
On Tue, 22 Oct 2024 17:52:42 +0200 Stefan Wahren wrote:
> -static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
> +static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff **txp,
>  				unsigned int pad)
>  {
>  	struct mse102x_net_spi *mses = to_mse102x_spi(mse);
> @@ -226,29 +226,29 @@ static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
>  	int ret;
> 
>  	netif_dbg(mse, tx_queued, mse->ndev, "%s: skb %p, %d@%p\n",
> -		  __func__, txp, txp->len, txp->data);
> +		  __func__, *txp, (*txp)->len, (*txp)->data);
> 
> -	if ((skb_headroom(txp) < DET_SOF_LEN) ||
> -	    (skb_tailroom(txp) < DET_DFT_LEN + pad)) {
> -		tskb = skb_copy_expand(txp, DET_SOF_LEN, DET_DFT_LEN + pad,
> +	if ((skb_headroom(*txp) < DET_SOF_LEN) ||
> +	    (skb_tailroom(*txp) < DET_DFT_LEN + pad)) {
> +		tskb = skb_copy_expand(*txp, DET_SOF_LEN, DET_DFT_LEN + pad,
>  				       GFP_KERNEL);
>  		if (!tskb)
>  			return -ENOMEM;
> 
> -		dev_kfree_skb(txp);
> -		txp = tskb;
> +		dev_kfree_skb(*txp);
> +		*txp = tskb;
>  	}
> 
> -	mse102x_push_header(txp);
> +	mse102x_push_header(*txp);
> 
>  	if (pad)
> -		skb_put_zero(txp, pad);
> +		skb_put_zero(*txp, pad);
> 
> -	mse102x_put_footer(txp);
> +	mse102x_put_footer(*txp);
> 
> -	xfer->tx_buf = txp->data;
> +	xfer->tx_buf = (*txp)->data;
>  	xfer->rx_buf = NULL;
> -	xfer->len = txp->len;
> +	xfer->len = (*txp)->len;
> 
>  	ret = spi_sync(mses->spidev, msg);
>  	if (ret < 0) {
> @@ -368,7 +368,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>  	mse->ndev->stats.rx_bytes += rxlen;

Isn't it easier to change this function to free the copy rather than
the original? That way the original will remain valid for the callers.
Re: [PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb
Posted by Stefan Wahren 3 weeks, 6 days ago 
Hi Jakub,

Am 29.10.24 um 20:10 schrieb Jakub Kicinski:
> On Tue, 22 Oct 2024 17:52:42 +0200 Stefan Wahren wrote:
>> -static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
>> +static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff **txp,
>>   				unsigned int pad)
>>   {
>>   	struct mse102x_net_spi *mses = to_mse102x_spi(mse);
>> @@ -226,29 +226,29 @@ static int mse102x_tx_frame_spi(struct mse102x_net *mse, struct sk_buff *txp,
>>   	int ret;
>>
>>   	netif_dbg(mse, tx_queued, mse->ndev, "%s: skb %p, %d@%p\n",
>> -		  __func__, txp, txp->len, txp->data);
>> +		  __func__, *txp, (*txp)->len, (*txp)->data);
>>
>> -	if ((skb_headroom(txp) < DET_SOF_LEN) ||
>> -	    (skb_tailroom(txp) < DET_DFT_LEN + pad)) {
>> -		tskb = skb_copy_expand(txp, DET_SOF_LEN, DET_DFT_LEN + pad,
>> +	if ((skb_headroom(*txp) < DET_SOF_LEN) ||
>> +	    (skb_tailroom(*txp) < DET_DFT_LEN + pad)) {
>> +		tskb = skb_copy_expand(*txp, DET_SOF_LEN, DET_DFT_LEN + pad,
>>   				       GFP_KERNEL);
>>   		if (!tskb)
>>   			return -ENOMEM;
>>
>> -		dev_kfree_skb(txp);
>> -		txp = tskb;
>> +		dev_kfree_skb(*txp);
>> +		*txp = tskb;
>>   	}
>>
>> -	mse102x_push_header(txp);
>> +	mse102x_push_header(*txp);
>>
>>   	if (pad)
>> -		skb_put_zero(txp, pad);
>> +		skb_put_zero(*txp, pad);
>>
>> -	mse102x_put_footer(txp);
>> +	mse102x_put_footer(*txp);
>>
>> -	xfer->tx_buf = txp->data;
>> +	xfer->tx_buf = (*txp)->data;
>>   	xfer->rx_buf = NULL;
>> -	xfer->len = txp->len;
>> +	xfer->len = (*txp)->len;
>>
>>   	ret = spi_sync(mses->spidev, msg);
>>   	if (ret < 0) {
>> @@ -368,7 +368,7 @@ static void mse102x_rx_pkt_spi(struct mse102x_net *mse)
>>   	mse->ndev->stats.rx_bytes += rxlen;
> Isn't it easier to change this function to free the copy rather than
> the original? That way the original will remain valid for the callers.
You mean something like this?

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;
  }
Re: [PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb
Posted by Jakub Kicinski 3 weeks, 6 days ago 
On Tue, 29 Oct 2024 22:15:15 +0100 Stefan Wahren wrote:
> > Isn't it easier to change this function to free the copy rather than
> > the original? That way the original will remain valid for the callers.  
> You mean something like this?
> 
> 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;
>   }

Exactly, I think it would work and it feels simpler.
Re: [PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb
Posted by Stefan Wahren 3 weeks, 6 days ago 
Am 29.10.24 um 23:01 schrieb Jakub Kicinski:
> On Tue, 29 Oct 2024 22:15:15 +0100 Stefan Wahren wrote:
>>> Isn't it easier to change this function to free the copy rather than
>>> the original? That way the original will remain valid for the callers.
>> You mean something like this?
>>
>> 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;
>>    }
> Exactly, I think it would work and it feels simpler.
I didn't test it yet, i need access to evaluation board before. But this
change will behave differently regarding stats of tx_bytes [1]. The
first version will include the padding, while the second does not.

[1] -
https://elixir.bootlin.com/linux/v6.12-rc5/source/drivers/net/ethernet/vertexcom/mse102x.c#L445
Re: [PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb
Posted by Jakub Kicinski 3 weeks, 5 days ago 
On Wed, 30 Oct 2024 00:06:46 +0100 Stefan Wahren wrote:
> > Exactly, I think it would work and it feels simpler.  
> I didn't test it yet, i need access to evaluation board before. But this
> change will behave differently regarding stats of tx_bytes [1]. The
> first version will include the padding, while the second does not.

Good point! But I think we'd be moving in the right direction.
tx_bytes should count bytes sent to the network, not data+metadata
sent on an internal bus.

If you connect this board to a different controller directly the
rx_bytes of the other end should match the tx_bytes of the board 
with mse102x. The byte accounting would benefit from further massaging
in a separate patch.
Re: [PATCH net] net: vertexcom: mse102x: Fix possible double free of TX skb
Posted by Simon Horman 1 month ago 
On Tue, Oct 22, 2024 at 05:52:42PM +0200, Stefan Wahren wrote:
> 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, also its pointer
> needs to be adjusted. Otherwise the already freed 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>

Reviewed-by: Simon Horman <horms@kernel.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.