| Message ID | 20220804181340.365429-1-ajay.kathat@microchip.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] wifi: wilc1000: fix DMA on stack objects | expand |
Hi, Am 2022-08-04 20:13, schrieb Ajay.Kathat@microchip.com: > From: Ajay Singh <ajay.kathat@microchip.com> > > Sometimes 'wilc_sdio_cmd53' is called with addresses pointing to an > object on the stack. Use dynamically allocated memory for cmd53 instead > of stack address which is not DMA'able. > > Fixes: 5625f965d764 ("wilc1000: move wilc driver out of staging") > Reported-by: Michael Walle <mwalle@kernel.org> > Suggested-by: Michael Walle <mwalle@kernel.org> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > --- > This patch is created based on [1] and changes are done as discussed in > the same thread. > > [1]. > https://patchwork.kernel.org/project/linux-wireless/patch/20220728152037.386543-1-michael@walle.cc/ > > changes since v1: > - add 'use_global_buf' variable to know when to use bounce > buffer > - remove unnecessary goto label > - dynamically allocate 'vmm_table' > > .../net/wireless/microchip/wilc1000/netdev.h | 1 + > .../net/wireless/microchip/wilc1000/sdio.c | 35 +++++++++++++++---- > .../net/wireless/microchip/wilc1000/wlan.c | 15 ++++++-- > 3 files changed, 43 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h > b/drivers/net/wireless/microchip/wilc1000/netdev.h > index 43c085c74b7a..bb1a315a7b7e 100644 > --- a/drivers/net/wireless/microchip/wilc1000/netdev.h > +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h > @@ -245,6 +245,7 @@ struct wilc { > u8 *rx_buffer; > u32 rx_buffer_offset; > u8 *tx_buffer; > + u32 *vmm_table; > > struct txq_handle txq[NQUEUES]; > int txq_entries; > diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c > b/drivers/net/wireless/microchip/wilc1000/sdio.c > index 600cc57e9da2..b12f411aec06 100644 > --- a/drivers/net/wireless/microchip/wilc1000/sdio.c > +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c > @@ -28,6 +28,7 @@ struct wilc_sdio { > u32 block_size; > bool isinit; > int has_thrpt_enh3; > + u8 *cmd53_buf; > }; > > struct sdio_cmd52 { > @@ -47,6 +48,7 @@ struct sdio_cmd53 { > u32 count: 9; > u8 *buffer; > u32 block_size; > + u8 use_global_buf; bool > }; > > static const struct wilc_hif_func wilc_hif_sdio; > @@ -91,6 +93,8 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct > sdio_cmd53 *cmd) > { > struct sdio_func *func = container_of(wilc->dev, struct sdio_func, > dev); > int size, ret; > + struct wilc_sdio *sdio_priv = wilc->bus_data; > + u8 *buf = cmd->buffer; > > sdio_claim_host(func); > > @@ -101,12 +105,19 @@ static int wilc_sdio_cmd53(struct wilc *wilc, > struct sdio_cmd53 *cmd) > else > size = cmd->count; > > + if (cmd->use_global_buf) > + buf = sdio_priv->cmd53_buf; There is no check if the size fits into the buffer. So maybe: if (size > sizeof(u32)) return -EINVAL; > + > if (cmd->read_write) { /* write */ > - ret = sdio_memcpy_toio(func, cmd->address, > - (void *)cmd->buffer, size); > + if (cmd->use_global_buf) > + memcpy(buf, cmd->buffer, size); > + > + ret = sdio_memcpy_toio(func, cmd->address, buf, size); > } else { /* read */ > - ret = sdio_memcpy_fromio(func, (void *)cmd->buffer, > - cmd->address, size); > + ret = sdio_memcpy_fromio(func, buf, cmd->address, size); > + > + if (cmd->use_global_buf) > + memcpy(cmd->buffer, buf, size); > } > > sdio_release_host(func); > @@ -128,6 +139,12 @@ static int wilc_sdio_probe(struct sdio_func *func, > if (!sdio_priv) > return -ENOMEM; > > + sdio_priv->cmd53_buf = kzalloc(sizeof(u32), GFP_KERNEL); > + if (!sdio_priv->cmd53_buf) { > + ret = -ENOMEM; > + goto free; > + } > + > ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO, > &wilc_hif_sdio); > if (ret) > @@ -161,6 +178,7 @@ static int wilc_sdio_probe(struct sdio_func *func, > irq_dispose_mapping(wilc->dev_irq_num); > wilc_netdev_cleanup(wilc); > free: > + kfree(sdio_priv->cmd53_buf); > kfree(sdio_priv); > return ret; > } > @@ -172,6 +190,7 @@ static void wilc_sdio_remove(struct sdio_func > *func) > > clk_disable_unprepare(wilc->rtc_clk); > wilc_netdev_cleanup(wilc); > + kfree(sdio_priv->cmd53_buf); > kfree(sdio_priv); > } > > @@ -375,8 +394,9 @@ static int wilc_sdio_write_reg(struct wilc *wilc, > u32 addr, u32 data) > cmd.address = WILC_SDIO_FBR_DATA_REG; > cmd.block_mode = 0; > cmd.increment = 1; > - cmd.count = 4; > + cmd.count = sizeof(u32); > cmd.buffer = (u8 *)&data; > + cmd.use_global_buf = 1; true > cmd.block_size = sdio_priv->block_size; > ret = wilc_sdio_cmd53(wilc, &cmd); > if (ret) > @@ -414,6 +434,7 @@ static int wilc_sdio_write(struct wilc *wilc, u32 > addr, u8 *buf, u32 size) > nblk = size / block_size; > nleft = size % block_size; > > + cmd.use_global_buf = 0; > if (nblk > 0) { > cmd.block_mode = 1; > cmd.increment = 1; > @@ -492,8 +513,9 @@ static int wilc_sdio_read_reg(struct wilc *wilc, > u32 addr, u32 *data) > cmd.address = WILC_SDIO_FBR_DATA_REG; > cmd.block_mode = 0; > cmd.increment = 1; > - cmd.count = 4; > + cmd.count = sizeof(u32); > cmd.buffer = (u8 *)data; > + cmd.use_global_buf = 1; true > > cmd.block_size = sdio_priv->block_size; > ret = wilc_sdio_cmd53(wilc, &cmd); > @@ -535,6 +557,7 @@ static int wilc_sdio_read(struct wilc *wilc, u32 > addr, u8 *buf, u32 size) > nblk = size / block_size; > nleft = size % block_size; > > + cmd.use_global_buf = 0; false > if (nblk > 0) { > cmd.block_mode = 1; > cmd.increment = 1; > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c > b/drivers/net/wireless/microchip/wilc1000/wlan.c > index 947d9a0a494e..58bbf50081e4 100644 > --- a/drivers/net/wireless/microchip/wilc1000/wlan.c > +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c > @@ -714,7 +714,7 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 > *txq_count) > int ret = 0; > int counter; > int timeout; > - u32 vmm_table[WILC_VMM_TBL_SIZE]; > + u32 *vmm_table = wilc->vmm_table; > u8 ac_pkt_num_to_chip[NQUEUES] = {0, 0, 0, 0}; > const struct wilc_hif_func *func; > int srcu_idx; > @@ -1252,6 +1252,8 @@ 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); > @@ -1489,6 +1491,14 @@ 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); > > @@ -1513,7 +1523,8 @@ 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);
Hi Michael, On 05/08/22 13:36, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know > the content is safe > > Hi, > > Am 2022-08-04 20:13, schrieb Ajay.Kathat@microchip.com: >> From: Ajay Singh <ajay.kathat@microchip.com> >> >> Sometimes 'wilc_sdio_cmd53' is called with addresses pointing to an >> object on the stack. Use dynamically allocated memory for cmd53 instead >> of stack address which is not DMA'able. >> >> Fixes: 5625f965d764 ("wilc1000: move wilc driver out of staging") >> Reported-by: Michael Walle <mwalle@kernel.org> >> Suggested-by: Michael Walle <mwalle@kernel.org> >> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> >> --- >> This patch is created based on [1] and changes are done as discussed in >> the same thread. >> >> [1]. >> https://patchwork.kernel.org/project/linux-wireless/patch/20220728152037.386543-1-michael@walle.cc/ >> >> >> changes since v1: >> - add 'use_global_buf' variable to know when to use bounce >> buffer >> - remove unnecessary goto label >> - dynamically allocate 'vmm_table' >> >> .../net/wireless/microchip/wilc1000/netdev.h | 1 + >> .../net/wireless/microchip/wilc1000/sdio.c | 35 +++++++++++++++---- >> .../net/wireless/microchip/wilc1000/wlan.c | 15 ++++++-- >> 3 files changed, 43 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h >> b/drivers/net/wireless/microchip/wilc1000/netdev.h >> index 43c085c74b7a..bb1a315a7b7e 100644 >> --- a/drivers/net/wireless/microchip/wilc1000/netdev.h >> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h >> @@ -245,6 +245,7 @@ struct wilc { >> u8 *rx_buffer; >> u32 rx_buffer_offset; >> u8 *tx_buffer; >> + u32 *vmm_table; >> >> struct txq_handle txq[NQUEUES]; >> int txq_entries; >> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c >> b/drivers/net/wireless/microchip/wilc1000/sdio.c >> index 600cc57e9da2..b12f411aec06 100644 >> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c >> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c >> @@ -28,6 +28,7 @@ struct wilc_sdio { >> u32 block_size; >> bool isinit; >> int has_thrpt_enh3; >> + u8 *cmd53_buf; >> }; >> >> struct sdio_cmd52 { >> @@ -47,6 +48,7 @@ struct sdio_cmd53 { >> u32 count: 9; >> u8 *buffer; >> u32 block_size; >> + u8 use_global_buf; > > bool > Ok. >> }; >> >> static const struct wilc_hif_func wilc_hif_sdio; >> @@ -91,6 +93,8 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct >> sdio_cmd53 *cmd) >> { >> struct sdio_func *func = container_of(wilc->dev, struct sdio_func, >> dev); >> int size, ret; >> + struct wilc_sdio *sdio_priv = wilc->bus_data; >> + u8 *buf = cmd->buffer; >> >> sdio_claim_host(func); >> >> @@ -101,12 +105,19 @@ static int wilc_sdio_cmd53(struct wilc *wilc, >> struct sdio_cmd53 *cmd) >> else >> size = cmd->count; >> >> + if (cmd->use_global_buf) >> + buf = sdio_priv->cmd53_buf; > > There is no check if the size fits into the buffer. So maybe: > > if (size > sizeof(u32)) > return -EINVAL; > Sure, I will make the changes and send the updated patch. Regards, Ajay
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h index 43c085c74b7a..bb1a315a7b7e 100644 --- a/drivers/net/wireless/microchip/wilc1000/netdev.h +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h @@ -245,6 +245,7 @@ struct wilc { u8 *rx_buffer; u32 rx_buffer_offset; u8 *tx_buffer; + u32 *vmm_table; struct txq_handle txq[NQUEUES]; int txq_entries; diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c index 600cc57e9da2..b12f411aec06 100644 --- a/drivers/net/wireless/microchip/wilc1000/sdio.c +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c @@ -28,6 +28,7 @@ struct wilc_sdio { u32 block_size; bool isinit; int has_thrpt_enh3; + u8 *cmd53_buf; }; struct sdio_cmd52 { @@ -47,6 +48,7 @@ struct sdio_cmd53 { u32 count: 9; u8 *buffer; u32 block_size; + u8 use_global_buf; }; static const struct wilc_hif_func wilc_hif_sdio; @@ -91,6 +93,8 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd) { struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev); int size, ret; + struct wilc_sdio *sdio_priv = wilc->bus_data; + u8 *buf = cmd->buffer; sdio_claim_host(func); @@ -101,12 +105,19 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd) else size = cmd->count; + if (cmd->use_global_buf) + buf = sdio_priv->cmd53_buf; + if (cmd->read_write) { /* write */ - ret = sdio_memcpy_toio(func, cmd->address, - (void *)cmd->buffer, size); + if (cmd->use_global_buf) + memcpy(buf, cmd->buffer, size); + + ret = sdio_memcpy_toio(func, cmd->address, buf, size); } else { /* read */ - ret = sdio_memcpy_fromio(func, (void *)cmd->buffer, - cmd->address, size); + ret = sdio_memcpy_fromio(func, buf, cmd->address, size); + + if (cmd->use_global_buf) + memcpy(cmd->buffer, buf, size); } sdio_release_host(func); @@ -128,6 +139,12 @@ static int wilc_sdio_probe(struct sdio_func *func, if (!sdio_priv) return -ENOMEM; + sdio_priv->cmd53_buf = kzalloc(sizeof(u32), GFP_KERNEL); + if (!sdio_priv->cmd53_buf) { + ret = -ENOMEM; + goto free; + } + ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO, &wilc_hif_sdio); if (ret) @@ -161,6 +178,7 @@ static int wilc_sdio_probe(struct sdio_func *func, irq_dispose_mapping(wilc->dev_irq_num); wilc_netdev_cleanup(wilc); free: + kfree(sdio_priv->cmd53_buf); kfree(sdio_priv); return ret; } @@ -172,6 +190,7 @@ static void wilc_sdio_remove(struct sdio_func *func) clk_disable_unprepare(wilc->rtc_clk); wilc_netdev_cleanup(wilc); + kfree(sdio_priv->cmd53_buf); kfree(sdio_priv); } @@ -375,8 +394,9 @@ static int wilc_sdio_write_reg(struct wilc *wilc, u32 addr, u32 data) cmd.address = WILC_SDIO_FBR_DATA_REG; cmd.block_mode = 0; cmd.increment = 1; - cmd.count = 4; + cmd.count = sizeof(u32); cmd.buffer = (u8 *)&data; + cmd.use_global_buf = 1; cmd.block_size = sdio_priv->block_size; ret = wilc_sdio_cmd53(wilc, &cmd); if (ret) @@ -414,6 +434,7 @@ static int wilc_sdio_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size) nblk = size / block_size; nleft = size % block_size; + cmd.use_global_buf = 0; if (nblk > 0) { cmd.block_mode = 1; cmd.increment = 1; @@ -492,8 +513,9 @@ static int wilc_sdio_read_reg(struct wilc *wilc, u32 addr, u32 *data) cmd.address = WILC_SDIO_FBR_DATA_REG; cmd.block_mode = 0; cmd.increment = 1; - cmd.count = 4; + cmd.count = sizeof(u32); cmd.buffer = (u8 *)data; + cmd.use_global_buf = 1; cmd.block_size = sdio_priv->block_size; ret = wilc_sdio_cmd53(wilc, &cmd); @@ -535,6 +557,7 @@ static int wilc_sdio_read(struct wilc *wilc, u32 addr, u8 *buf, u32 size) nblk = size / block_size; nleft = size % block_size; + cmd.use_global_buf = 0; if (nblk > 0) { cmd.block_mode = 1; cmd.increment = 1; diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c index 947d9a0a494e..58bbf50081e4 100644 --- a/drivers/net/wireless/microchip/wilc1000/wlan.c +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c @@ -714,7 +714,7 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count) int ret = 0; int counter; int timeout; - u32 vmm_table[WILC_VMM_TBL_SIZE]; + u32 *vmm_table = wilc->vmm_table; u8 ac_pkt_num_to_chip[NQUEUES] = {0, 0, 0, 0}; const struct wilc_hif_func *func; int srcu_idx; @@ -1252,6 +1252,8 @@ 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); @@ -1489,6 +1491,14 @@ 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); @@ -1513,7 +1523,8 @@ 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);