[PATCH v2] wifi: wilc1000: use vmm_table as array in wilc struct

[PATCH v2] wifi: wilc1000: use vmm_table as array in wilc struct

[Alexis Lothoré]

From: Ajay Singh <ajay.kathat@microchip.com>

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: stable@vger.kernel.org
Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Changes in v2:
- keep dedicated dynamic allocation for vmm_table
- Link to v1: https://lore.kernel.org/r/20231013-wilc1000_tx_oops-v1-1-3761beb9524d@bootlin.com
---
 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

Re: [PATCH v2] wifi: wilc1000: use vmm_table as array in wilc struct

[Michael Walle]

Am 2023-10-16 10:29, schrieb Alexis Lothoré:
> From: Ajay Singh <ajay.kathat@microchip.com>
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

Reviewed-by: Michael Walle <mwalle@kernel.org>

-michael

Re: [PATCH v2] wifi: wilc1000: use vmm_table as array in wilc struct

[Jeff Johnson]

On 10/16/2023 1:29 AM, Alexis Lothoré wrote:
> From: Ajay Singh <ajay.kathat@microchip.com>
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> ---
> Changes in v2:
> - keep dedicated dynamic allocation for vmm_table
> - Link to v1: https://lore.kernel.org/r/20231013-wilc1000_tx_oops-v1-1-3761beb9524d@bootlin.com
> ---
>   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,

Re: [PATCH v2] wifi: wilc1000: use vmm_table as array in wilc struct

[Alexis Lothoré]

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

Re: [PATCH v2] wifi: wilc1000: use vmm_table as array in wilc struct

[Jeff Johnson]

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

Re: [PATCH v2] wifi: wilc1000: use vmm_table as array in wilc struct

[Alexis Lothoré]

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