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@bootli... --- 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,
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
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@bootli...
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
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
linux-stable-mirror@lists.linaro.org
-
Alexis Lothoré -
Jeff Johnson -
Michael Walle