Search Linux Wireless

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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 2022-07-29 17:39, schrieb Ajay.Kathat@xxxxxxxxxxxxx:
On 29/07/22 20:28, Michael Walle wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Am 29. Juli 2022 11:51:12 MESZ schrieb David Laight <David.Laight@xxxxxxxxxx>:
From: Michael Walle
Sent: 28 July 2022 16:21

From: Michael Walle <mwalle@xxxxxxxxxx>

Sometimes wilc_sdio_cmd53() is called with addresses pointing to an
object on the stack. E.g. wilc_sdio_write_reg() will call it with an
address pointing to one of its arguments. Detect whether the buffer
address is not DMA-able in which case a bounce buffer is used. The bounce buffer itself is protected from parallel accesses by sdio_claim_host().

Fixes: 5625f965d764 ("wilc1000: move wilc driver out of staging")
Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
---
The bug itself probably goes back way more, but I don't know if it makes any sense to use an older commit for the Fixes tag. If so, please suggest
one.

The bug leads to an actual error on an imx8mn SoC with 1GiB of RAM. But the
error will also be catched by CONFIG_DEBUG_VIRTUAL:
[ 9.817512] virt_to_phys used for non-linear address: (____ptrval____) (0xffff80000a94bc9c)

.../net/wireless/microchip/wilc1000/sdio.c | 28 ++++++++++++++++---
  1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c
b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 7962c11cfe84..e988bede880c 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -27,6 +27,7 @@ struct wilc_sdio {
      bool irq_gpio;
      u32 block_size;
      int has_thrpt_enh3;
+    u8 *dma_buffer;
  };

  struct sdio_cmd52 {
@@ -89,6 +90,9 @@ static int wilc_sdio_cmd52(struct wilc *wilc, struct sdio_cmd52 *cmd) static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
  {
struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev);
+    struct wilc_sdio *sdio_priv = wilc->bus_data;
+    bool need_bounce_buf = false;
+    u8 *buf = cmd->buffer;
      int size, ret;

      sdio_claim_host(func);
@@ -100,12 +104,20 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
      else
              size = cmd->count;

+    if ((!virt_addr_valid(buf) || object_is_on_stack(buf)) &&
How cheap are the above tests?
It might just be worth always doing the 'bounce'?
I'm not sure how cheap they are, but I don't think it costs more than copying the bulk data around. That's up to the maintainer to decide.


I think, the above checks for each CMD53 might add up to the processing
time of this function. These checks can be avoided, if we add new
function similar to 'wilc_sdio_cmd53' which can be called when the local variables are used. Though we have to perform the memcpy operation which
is anyway required to handle this scenario for small size data.

Mostly, either the static global data or dynamically allocated buffer is
used with cmd53 except wilc_sdio_write_reg, wilc_sdio_read_reg
wilc_wlan_handle_txq functions.

I have created a patch using the above approach which can fix this issue
and will have no or minimal impact on existing functionality. The same
is copied below:


---
  .../net/wireless/microchip/wilc1000/netdev.h  |  1 +
 .../net/wireless/microchip/wilc1000/sdio.c    | 46 +++++++++++++++++--
  .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
  3 files changed, 45 insertions(+), 4 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..19d4350ecc22 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 *dma_buffer;
  };

  struct sdio_cmd52 {
@@ -117,6 +118,36 @@ static int wilc_sdio_cmd53(struct wilc *wilc,
struct sdio_cmd53 *cmd)
      return ret;
  }

+static int wilc_sdio_cmd53_extend(struct wilc *wilc, struct sdio_cmd53
*cmd)

If you handle all the stack cases anyway, the caller can just use
a bounce buffer and you don't need to duplicate the function.

-michael



[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux