| Message ID | 20231016-wilc1000_tx_oops-v2-1-8d1982a29ef1@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] wifi: wilc1000: use vmm_table as array in wilc struct | expand |
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@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,
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
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;