| Message ID | 20241022155242.33729-1-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: vertexcom: mse102x: Fix possible double free of TX skb | expand |
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>
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.
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; }
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.
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
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.
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++;
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(-) -- 2.34.1