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

[Michael Walle <michael@xxxxxxxx>]

Hi,

thanks for the patch!

Am 2022-08-04 15:18, schrieb Ajay.Kathat@xxxxxxxxxxxxx:
> From: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Suggested-by: Michael Walle <mwalle@xxxxxxxxxx>
> Signed-off-by: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxx/
>
>  .../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;