| 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: <c7bf83a2-9419-5e0d-25ec-82c80ad319f4@microchip.com>
Date: Thu, 4 Aug 2022 13:22:49 +0000
From: <Ajay.Kathat@...rochip.com>
To: <michael@...le.cc>
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
On 04/08/22 18:26, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
>
> Am 2022-08-04 14:43, schrieb Ajay.Kathat@...rochip.com:
>> On 04/08/22 12:52, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> 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.
>>
>>
>> Thanks. Indeed, the duplicate function can be avoided. I will update
>> the
>> patch and send modified patch for the review.
>> Btw, I was trying to reproduce the warning message by enabling
>> CONFIG_DEBUG_VIRTUAL config but no luck. It seems enabling the config
>> is
>> not enough to test on my host or may be I am missing something.
>
> Did you bring the interface up?
Yes, I tested by bringing the interface up on SAMA5D4 Xplained host.
>
>> I would
>> need the help to test and confirm if the modified patch do solve the
>> issue with imx8mn.
>
> sure, just put me on cc and i can test it on my board.
Sure. Thanks.
Regards,
Ajay
Powered by blists - more mailing lists