From: Ajay Singh <[email protected]>
Enabling KASAN and running some iperf tests raises some memory issues with
vmm_table:
BUG: KASAN: slab-out-of-bounds in wilc_wlan_handle_txq+0x6ac/0xdb4
Write of size 4 at addr c3a61540 by task wlan0-tx/95
KASAN detects that we are writing data beyond range allocated to vmm_table.
There is indeed a mismatch between the size passed to allocator in
wilc_wlan_init, and the range of possible indexes used later: allocation
size is missing a multiplication by sizeof(u32)
Fixes: 40b717bfcefa ("wifi: wilc1000: fix DMA on stack objects")
Cc: [email protected]
Signed-off-by: Ajay Singh <[email protected]>
Signed-off-by: Alexis Lothoré <[email protected]>
---
Changes in v2:
- keep dedicated dynamic allocation for vmm_table
- Link to v1: https://lore.kernel.org/r/[email protected]
---
drivers/net/wireless/microchip/wilc1000/wlan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 58bbf50081e4..e4113f2dfadf 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1492,7 +1492,7 @@ int wilc_wlan_init(struct net_device *dev)
}
if (!wilc->vmm_table)
- wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE, GFP_KERNEL);
+ wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE * sizeof(u32), GFP_KERNEL);
if (!wilc->vmm_table) {
ret = -ENOBUFS;
---
base-commit: ea12d85cbfd6b08fff40a4fefccc011b6cfadf8e
change-id: 20231012-wilc1000_tx_oops-58ce91ee3e93
Best regards,
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Am 2023-10-16 10:29, schrieb Alexis Lothoré:
> From: Ajay Singh <[email protected]>
>
> Enabling KASAN and running some iperf tests raises some memory issues
> with
> vmm_table:
>
> BUG: KASAN: slab-out-of-bounds in wilc_wlan_handle_txq+0x6ac/0xdb4
> Write of size 4 at addr c3a61540 by task wlan0-tx/95
>
> KASAN detects that we are writing data beyond range allocated to
> vmm_table.
> There is indeed a mismatch between the size passed to allocator in
> wilc_wlan_init, and the range of possible indexes used later:
> allocation
> size is missing a multiplication by sizeof(u32)
>
> Fixes: 40b717bfcefa ("wifi: wilc1000: fix DMA on stack objects")
> Cc: [email protected]
> Signed-off-by: Ajay Singh <[email protected]>
> Signed-off-by: Alexis Lothoré <[email protected]>
Reviewed-by: Michael Walle <[email protected]>
-michael
On 10/16/2023 1:29 AM, Alexis Lothoré wrote:
> From: Ajay Singh <[email protected]>
>
> Enabling KASAN and running some iperf tests raises some memory issues with
> vmm_table:
>
> BUG: KASAN: slab-out-of-bounds in wilc_wlan_handle_txq+0x6ac/0xdb4
> Write of size 4 at addr c3a61540 by task wlan0-tx/95
>
> KASAN detects that we are writing data beyond range allocated to vmm_table.
> There is indeed a mismatch between the size passed to allocator in
> wilc_wlan_init, and the range of possible indexes used later: allocation
> size is missing a multiplication by sizeof(u32)
>
> Fixes: 40b717bfcefa ("wifi: wilc1000: fix DMA on stack objects")
> Cc: [email protected]
> Signed-off-by: Ajay Singh <[email protected]>
> Signed-off-by: Alexis Lothoré <[email protected]>
> ---
> Changes in v2:
> - keep dedicated dynamic allocation for vmm_table
> - Link to v1: https://lore.kernel.org/r/[email protected]
> ---
> drivers/net/wireless/microchip/wilc1000/wlan.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 58bbf50081e4..e4113f2dfadf 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -1492,7 +1492,7 @@ int wilc_wlan_init(struct net_device *dev)
> }
>
> if (!wilc->vmm_table)
> - wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE, GFP_KERNEL);
> + wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE * sizeof(u32), GFP_KERNEL);
this is probably OK since the values are constant, but kcalloc() is
generally preferred
>
> if (!wilc->vmm_table) {
> ret = -ENOBUFS;
>
> ---
> base-commit: ea12d85cbfd6b08fff40a4fefccc011b6cfadf8e
> change-id: 20231012-wilc1000_tx_oops-58ce91ee3e93
>
> Best regards,
Hello Jeff,
On 10/16/23 17:26, Jeff Johnson wrote:
> On 10/16/2023 1:29 AM, Alexis Lothoré wrote:
>> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c
>> b/drivers/net/wireless/microchip/wilc1000/wlan.c
>> index 58bbf50081e4..e4113f2dfadf 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
>> @@ -1492,7 +1492,7 @@ int wilc_wlan_init(struct net_device *dev)
>> }
>> if (!wilc->vmm_table)
>> - wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE, GFP_KERNEL);
>> + wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE * sizeof(u32), GFP_KERNEL);
>
> this is probably OK since the values are constant, but kcalloc() is generally
> preferred
Ok, I can submit a new version with kcalloc. One thing that I do not understand
however is why checkpatch.pl remains silent on this one. I guess it should raise
the ALLOC_WITH_MULTIPLY warning here. I tried to dive into the script to
understand why, but I drowned in regexes (and Perl, with which I am not familiar
with). Could it be because of both sides being constant ?
Thanks,
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 10/16/2023 2:23 PM, Alexis Lothoré wrote:
> Hello Jeff,
>
> On 10/16/23 17:26, Jeff Johnson wrote:
>> On 10/16/2023 1:29 AM, Alexis Lothoré wrote:
>>> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c
>>> b/drivers/net/wireless/microchip/wilc1000/wlan.c
>>> index 58bbf50081e4..e4113f2dfadf 100644
>>> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
>>> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
>>> @@ -1492,7 +1492,7 @@ int wilc_wlan_init(struct net_device *dev)
>>> }
>>> if (!wilc->vmm_table)
>>> - wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE, GFP_KERNEL);
>>> + wilc->vmm_table = kzalloc(WILC_VMM_TBL_SIZE * sizeof(u32), GFP_KERNEL);
>>
>> this is probably OK since the values are constant, but kcalloc() is generally
>> preferred
>
> Ok, I can submit a new version with kcalloc. One thing that I do not understand
> however is why checkpatch.pl remains silent on this one. I guess it should raise
> the ALLOC_WITH_MULTIPLY warning here. I tried to dive into the script to
> understand why, but I drowned in regexes (and Perl, with which I am not familiar
> with). Could it be because of both sides being constant ?
I also drown when looking at checkpatch.pl -- so many "write-only"
regexes! But I think the following is what excludes your patch:
$r1 =~ /^[A-Z_][A-Z0-9_]*$
It is a compile-time constant so the compiler can flag on overflow, so
it's your call to modify or not.
/jeff
On 10/17/23 00:51, Jeff Johnson wrote:
> On 10/16/2023 2:23 PM, Alexis Lothoré wrote:
>> Hello Jeff,
>>
>> On 10/16/23 17:26, Jeff Johnson wrote:
>>> On 10/16/2023 1:29 AM, Alexis Lothoré wrote:
>>> this is probably OK since the values are constant, but kcalloc() is generally
>>> preferred
>>
>> Ok, I can submit a new version with kcalloc. One thing that I do not understand
>> however is why checkpatch.pl remains silent on this one. I guess it should raise
>> the ALLOC_WITH_MULTIPLY warning here. I tried to dive into the script to
>> understand why, but I drowned in regexes (and Perl, with which I am not familiar
>> with). Could it be because of both sides being constant ?
>
> I also drown when looking at checkpatch.pl -- so many "write-only" regexes! But
> I think the following is what excludes your patch:
> $r1 =~ /^[A-Z_][A-Z0-9_]*$
>
> It is a compile-time constant so the compiler can flag on overflow, so it's your
> call to modify or not.
Thanks for taking a look. I have tried to tweak those lines to see if it makes
checkpatch raise the warning, without success.
Anyway, I agree with your initial statement, let's keep the code base
homogeneous and replace kzalloc with kcalloc
> /jeff
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com