| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b40636e354df866d044c07241483ff81@walle.cc>
Date: Thu, 04 Aug 2022 09:22:51 +0200
From: Michael Walle <michael@...le.cc>
To: Ajay.Kathat@...rochip.com
Cc: David.Laight@...lab.com, Claudiu.Beznea@...rochip.com,
kvalo@...nel.org, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, linux-wireless@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
mwalle@...nel.org
Subject: Re: [PATCH] wilc1000: fix DMA on stack objects
Am 2022-07-29 17:39, schrieb Ajay.Kathat@...rochip.com:
> 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@...LAB.COM>:
>>> From: Michael Walle
>>>> Sent: 28 July 2022 16:21
>>>>
>>>> From: Michael Walle <mwalle@...nel.org>
>>>>
>>>> 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@...nel.org>
>>>> ---
>>>> 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
Powered by blists - more mailing lists