| Message ID | 20231013-wilc1000_tx_oops-v1-1-3761beb9524d@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: wilc1000: use vmm_table as array in wilc struct | expand |
Hi, Am 2023-10-13 10:26, schrieb Alexis Lothoré: > From: Ajay Singh <ajay.kathat@microchip.com> > > Enabling KASAN and running some iperf tests raises some memory issues > with > vmm_table: > > BUG: KASAN: slab-out-of-bounds in wilc_wlan_handle_txq+0x6ac/0xdb4 > Write of size 4 at addr c3a61540 by task wlan0-tx/95 > > KASAN detects that we are writing data beyond range allocated to > vmm_table. > There is indeed a mismatch between the size passed to allocator in > wilc_wlan_init, and the range of possible indexes used later: > allocation > size is missing a multiplication by sizeof(u32) Nice catch. > While at it, instead of simply multiplying the allocation size, do not > keep > dedicated dynamic allocation for vmm_table: define it as an array with > the > relevant size in wilc struct, which is already dynamically allocated > > Fixes: 40b717bfcefa ("wifi: wilc1000: fix DMA on stack objects") > Cc: stable@vger.kernel.org > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> Looks good to me. But you'll change the alignment of the table, not sure if that matters for some DMA controllers. -michael
Hello Michael, On 10/13/23 11:24, Michael Walle wrote: >> Fixes: 40b717bfcefa ("wifi: wilc1000: fix DMA on stack objects") >> Cc: stable@vger.kernel.org >> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> >> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> > > Looks good to me. But you'll change the alignment of the table, not sure > if that matters for some DMA controllers. Thanks for the review. Indeed, I may have overlooked that point. I am not sure either how much of an issue it could be, but checking back the driver history on lore, I see that it has already been mentioned, so let's be safe and keep a dedicated kzalloc for vmm_table to make sure its address is safe to use with DMA. I will send the corresponding v2. Thanks, Alexis > > -michael
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h index bb1a315a7b7e..2137ef294953 100644 --- a/drivers/net/wireless/microchip/wilc1000/netdev.h +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h @@ -245,7 +245,7 @@ struct wilc { u8 *rx_buffer; u32 rx_buffer_offset; u8 *tx_buffer; - u32 *vmm_table; + u32 vmm_table[WILC_VMM_TBL_SIZE]; struct txq_handle txq[NQUEUES]; int txq_entries; diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c index 58bbf50081e4..d93493c40e49 100644 --- a/drivers/net/wireless/microchip/wilc1000/wlan.c +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c @@ -1252,8 +1252,6 @@ void wilc_wlan_cleanup(struct net_device *dev) while ((rqe = wilc_wlan_rxq_remove(wilc))) kfree(rqe); - kfree(wilc->vmm_table); - wilc->vmm_table = NULL; kfree(wilc->rx_buffer); wilc->rx_buffer = NULL; kfree(wilc->tx_buffer); @@ -1491,14 +1489,6 @@ int wilc_wlan_init(struct net_device *dev) goto fail; } - if (!wilc->vmm_table) - wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE, GFP_KERNEL); - - if (!wilc->vmm_table) { - ret = -ENOBUFS; - goto fail; - } - if (!wilc->tx_buffer) wilc->tx_buffer = kmalloc(WILC_TX_BUFF_SIZE, GFP_KERNEL); @@ -1523,8 +1513,6 @@ int wilc_wlan_init(struct net_device *dev) return 0; fail: - kfree(wilc->vmm_table); - wilc->vmm_table = NULL; kfree(wilc->rx_buffer); wilc->rx_buffer = NULL; kfree(wilc->tx_buffer);