From: Ajay Singh <[email protected]>
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 <[email protected]>
Suggested-by: Michael Walle <[email protected]>
Signed-off-by: Ajay Singh <[email protected]>
---
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/[email protected]/
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;
};
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);
--
2.34.1
Hi,
Am 2022-08-04 20:13, schrieb [email protected]:
> From: Ajay Singh <[email protected]>
>
> 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 <[email protected]>
> Suggested-by: Michael Walle <[email protected]>
> Signed-off-by: Ajay Singh <[email protected]>
> ---
> 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/[email protected]/
>
> 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);
--
-michael
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 [email protected]:
>> From: Ajay Singh <[email protected]>
>>
>> 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 <[email protected]>
>> Suggested-by: Michael Walle <[email protected]>
>> Signed-off-by: Ajay Singh <[email protected]>
>> ---
>> 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/[email protected]/
>>
>>
>> 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