2022-08-04 13:27:54

by Ajay Singh

[permalink] [raw]
Subject: [PATCH] wifi: wilc1000: fix DMA on stack objects

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]/

.../net/wireless/microchip/wilc1000/netdev.h | 1 +
.../net/wireless/microchip/wilc1000/sdio.c | 23 +++++++++++++++----
.../net/wireless/microchip/wilc1000/wlan.c | 2 +-
3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index 43c085c74b7a..2137ef294953 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[WILC_VMM_TBL_SIZE];

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..8bb0b8e189af 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 {
@@ -128,10 +129,16 @@ 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)
- goto free;
+ goto free_buffer;

if (IS_ENABLED(CONFIG_WILC1000_HW_OOB_INTR)) {
struct device_node *np = func->card->dev.of_node;
@@ -160,6 +167,8 @@ static int wilc_sdio_probe(struct sdio_func *func,
dispose_irq:
irq_dispose_mapping(wilc->dev_irq_num);
wilc_netdev_cleanup(wilc);
+free_buffer:
+ kfree(sdio_priv->cmd53_buf);
free:
kfree(sdio_priv);
return ret;
@@ -172,6 +181,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 +385,10 @@ 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.buffer = (u8 *)&data;
+ cmd.count = sizeof(u32);
+ /* copy to a bounce buffer to avoid use of stack variable */
+ memcpy(sdio_priv->cmd53_buf, &data, sizeof(u32));
+ cmd.buffer = sdio_priv->cmd53_buf;
cmd.block_size = sdio_priv->block_size;
ret = wilc_sdio_cmd53(wilc, &cmd);
if (ret)
@@ -492,8 +504,8 @@ 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.buffer = (u8 *)data;
+ cmd.count = sizeof(u32);
+ cmd.buffer = sdio_priv->cmd53_buf;

cmd.block_size = sdio_priv->block_size;
ret = wilc_sdio_cmd53(wilc, &cmd);
@@ -502,6 +514,7 @@ static int wilc_sdio_read_reg(struct wilc *wilc, u32 addr, u32 *data)
"Failed cmd53, read reg (%08x)...\n", addr);
return ret;
}
+ memcpy(data, sdio_priv->cmd53_buf, sizeof(u32));
}

le32_to_cpus(data);
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 947d9a0a494e..0576d865c603 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;
--
2.34.1


2022-08-04 14:27:04

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] wifi: wilc1000: fix DMA on stack objects

Hi,

thanks for the patch!

Am 2022-08-04 15:18, 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]/
>
> .../net/wireless/microchip/wilc1000/netdev.h | 1 +
> .../net/wireless/microchip/wilc1000/sdio.c | 23 +++++++++++++++----
> .../net/wireless/microchip/wilc1000/wlan.c | 2 +-
> 3 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h
> b/drivers/net/wireless/microchip/wilc1000/netdev.h
> index 43c085c74b7a..2137ef294953 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[WILC_VMM_TBL_SIZE];

Shouldn't this be "u32 *vmm_table" judging by the
other members of this structure.

> 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..8bb0b8e189af 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 {
> @@ -128,10 +129,16 @@ 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)
> - goto free;

just use "goto free;". kfree(NULL) is a noop.

> + goto free_buffer;
>
> if (IS_ENABLED(CONFIG_WILC1000_HW_OOB_INTR)) {
> struct device_node *np = func->card->dev.of_node;
> @@ -160,6 +167,8 @@ static int wilc_sdio_probe(struct sdio_func *func,
> dispose_irq:
> irq_dispose_mapping(wilc->dev_irq_num);
> wilc_netdev_cleanup(wilc);
> +free_buffer:
> + kfree(sdio_priv->cmd53_buf);
> free:
> kfree(sdio_priv);
> return ret;
> @@ -172,6 +181,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 +385,10 @@ 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.buffer = (u8 *)&data;
> + cmd.count = sizeof(u32);
> + /* copy to a bounce buffer to avoid use of stack variable */
> + memcpy(sdio_priv->cmd53_buf, &data, sizeof(u32));

Locking seems to be missing, no? How is sdio_priv->cmd53_buf
protected from parallel access?

-michael

> + cmd.buffer = sdio_priv->cmd53_buf;
> cmd.block_size = sdio_priv->block_size;
> ret = wilc_sdio_cmd53(wilc, &cmd);
> if (ret)
> @@ -492,8 +504,8 @@ 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.buffer = (u8 *)data;
> + cmd.count = sizeof(u32);
> + cmd.buffer = sdio_priv->cmd53_buf;
>
> cmd.block_size = sdio_priv->block_size;
> ret = wilc_sdio_cmd53(wilc, &cmd);
> @@ -502,6 +514,7 @@ static int wilc_sdio_read_reg(struct wilc *wilc,
> u32 addr, u32 *data)
> "Failed cmd53, read reg (%08x)...\n", addr);
> return ret;
> }
> + memcpy(data, sdio_priv->cmd53_buf, sizeof(u32));
> }
>
> le32_to_cpus(data);
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c
> b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 947d9a0a494e..0576d865c603 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;

2022-08-04 18:14:24

by Ajay Singh

[permalink] [raw]
Subject: Re: [PATCH] wifi: wilc1000: fix DMA on stack objects

On 04/08/22 19:55, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
>
> Hi,
>
> thanks for the patch!
>
> Am 2022-08-04 15:18, 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 <mwalle[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]/
>>
>>
>>  .../net/wireless/microchip/wilc1000/netdev.h  |  1 +
>>  .../net/wireless/microchip/wilc1000/sdio.c    | 23 +++++++++++++++----
>>  .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>>  3 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h
>> b/drivers/net/wireless/microchip/wilc1000/netdev.h
>> index 43c085c74b7a..2137ef294953 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[WILC_VMM_TBL_SIZE];
>
> Shouldn't this be "u32 *vmm_table" judging by the
> other members of this structure.
>

Okay. I will change it.


>>       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..8bb0b8e189af 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 {
>> @@ -128,10 +129,16 @@ 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)
>> -             goto free;
>
> just use "goto free;". kfree(NULL) is a noop.
>
Ok

>> +             goto free_buffer;
>>
>>       if (IS_ENABLED(CONFIG_WILC1000_HW_OOB_INTR)) {
>>               struct device_node *np = func->card->dev.of_node;
>> @@ -160,6 +167,8 @@ static int wilc_sdio_probe(struct sdio_func *func,
>>  dispose_irq:
>>       irq_dispose_mapping(wilc->dev_irq_num);
>>       wilc_netdev_cleanup(wilc);
>> +free_buffer:
>> +     kfree(sdio_priv->cmd53_buf);
>>  free:
>>       kfree(sdio_priv);
>>       return ret;
>> @@ -172,6 +181,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 +385,10 @@ 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.buffer = (u8 *)&data;
>> +             cmd.count = sizeof(u32);
>> +             /* copy to a bounce buffer to avoid use of stack
>> variable */
>> +             memcpy(sdio_priv->cmd53_buf, &data, sizeof(u32));
>
> Locking seems to be missing, no? How is sdio_priv->cmd53_buf
> protected from parallel access?

Yes, I also think lock would be required with these changes. I am
planning to use the existing 'sdio_claim_host' locking instead of
introducing a new lock and also take care to not adding a duplicate API
to handle cmd53. For better understanding, I will send the v2 patch for
review.

Regards,
Ajay'