[PATCH wireless-drivers] mt76x0: Remove VLA usage

[PATCH wireless-drivers] mt76x0: Remove VLA usage

[Kees Cook]

Even with "const" variables, the compiler will generate warnings about
VLA usage. In the quest to remove all VLAs from the kernel[1], this uses
a #define instead of a const to do the array sizing.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Fixes: e87b5039511a ("mt76x0: eeprom files")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Please include this for the v4.19 merge window. The VLA was introduced
with the new source file (which I also note is missing a SPDX line), so
I'd like to avoid the kernel ever getting released with a VLA here.
---
 drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c b/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
index 1ecd018f12b8..af2fd6a1bb44 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
@@ -81,15 +81,15 @@ mt76x0_efuse_read(struct mt76x0_dev *dev, u16 addr, u8 *data,
 	return 0;
 }
 
+#define MT_MAP_READS	DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)
 static int
 mt76x0_efuse_physical_size_check(struct mt76x0_dev *dev)
 {
-	const int map_reads = DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16);
-	u8 data[map_reads * 16];
+	u8 data[MT_MAP_READS * 16];
 	int ret, i;
 	u32 start = 0, end = 0, cnt_free;
 
-	for (i = 0; i < map_reads; i++) {
+	for (i = 0; i < MT_MAP_READS; i++) {
 		ret = mt76x0_efuse_read(dev, MT_EE_USAGE_MAP_START + i * 16,
 					 data + i * 16, MT_EE_PHYSICAL_READ);
 		if (ret)
-- 
2.17.1


-- 
Kees Cook
Pixel Security

Re: [PATCH wireless-drivers] mt76x0: Remove VLA usage

[Kalle Valo]

Kees Cook <keescook@chromium.org> writes:

> Even with "const" variables, the compiler will generate warnings about
> VLA usage. In the quest to remove all VLAs from the kernel[1], this uses
> a #define instead of a const to do the array sizing.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Fixes: e87b5039511a ("mt76x0: eeprom files")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Please include this for the v4.19 merge window. The VLA was introduced
> with the new source file (which I also note is missing a SPDX line), so
> I'd like to avoid the kernel ever getting released with a VLA here.

Ok, I'll push this to v4.19.

-- 
Kalle Valo

Re: [PATCH wireless-drivers] mt76x0: Remove VLA usage

[Stanislaw Gruszka]

On Tue, Aug 07, 2018 at 03:50:40PM -0700, Kees Cook wrote:
> Even with "const" variables, the compiler will generate warnings about
> VLA usage. In the quest to remove all VLAs from the kernel[1], this uses
> a #define instead of a const to do the array sizing.
> 
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> 
> Fixes: e87b5039511a ("mt76x0: eeprom files")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Please include this for the v4.19 merge window. The VLA was introduced
> with the new source file (which I also note is missing a SPDX line), so

I thought SPDX line is needed only if file has no license and eeprom.c
file and other mt76x0 files have specified the license. Is SPDX still
needed in that case ?
 
> +#define MT_MAP_READS	DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)
>  static int
>  mt76x0_efuse_physical_size_check(struct mt76x0_dev *dev)
>  {
> -	const int map_reads = DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16);
> -	u8 data[map_reads * 16];

Why this is variable length array? DIV_ROUND_UP can not be calculated
at compile time? But if so, macro do not change the situation either.

Thanks
Stanislaw

Re: [PATCH wireless-drivers] mt76x0: Remove VLA usage

[Kalle Valo]

Stanislaw Gruszka <sgruszka@redhat.com> writes:

> On Tue, Aug 07, 2018 at 03:50:40PM -0700, Kees Cook wrote:
>> Even with "const" variables, the compiler will generate warnings about
>> VLA usage. In the quest to remove all VLAs from the kernel[1], this uses
>> a #define instead of a const to do the array sizing.
>> 
>> [1]
>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>> 
>> Fixes: e87b5039511a ("mt76x0: eeprom files")
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> Please include this for the v4.19 merge window. The VLA was introduced
>> with the new source file (which I also note is missing a SPDX line), so
>
> I thought SPDX line is needed only if file has no license and eeprom.c
> file and other mt76x0 files have specified the license. Is SPDX still
> needed in that case ?
>  
>> +#define MT_MAP_READS	DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)
>>  static int
>>  mt76x0_efuse_physical_size_check(struct mt76x0_dev *dev)
>>  {
>> -	const int map_reads = DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16);
>> -	u8 data[map_reads * 16];
>
> Why this is variable length array? DIV_ROUND_UP can not be calculated
> at compile time? But if so, macro do not change the situation either.

The commit log mentioned:

  "Even with "const" variables, the compiler will generate warnings about
   VLA usage."

So I guess the compiler (gcc?) is just not smart enough in this case?

-- 
Kalle Valo

Re: [PATCH wireless-drivers] mt76x0: Remove VLA usage

[Kees Cook]

On Wed, Aug 8, 2018 at 2:46 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Stanislaw Gruszka <sgruszka@redhat.com> writes:
>
>> On Tue, Aug 07, 2018 at 03:50:40PM -0700, Kees Cook wrote:
>>> Even with "const" variables, the compiler will generate warnings about
>>> VLA usage. In the quest to remove all VLAs from the kernel[1], this uses
>>> a #define instead of a const to do the array sizing.
>>>
>>> [1]
>>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>>
>>> Fixes: e87b5039511a ("mt76x0: eeprom files")
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> Please include this for the v4.19 merge window. The VLA was introduced
>>> with the new source file (which I also note is missing a SPDX line), so
>>
>> I thought SPDX line is needed only if file has no license and eeprom.c
>> file and other mt76x0 files have specified the license. Is SPDX still
>> needed in that case ?

I thought all source files needed SPDX: https://lwn.net/Articles/739183/

>>
>>> +#define MT_MAP_READS        DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)
>>>  static int
>>>  mt76x0_efuse_physical_size_check(struct mt76x0_dev *dev)
>>>  {
>>> -    const int map_reads = DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16);
>>> -    u8 data[map_reads * 16];
>>
>> Why this is variable length array? DIV_ROUND_UP can not be calculated
>> at compile time? But if so, macro do not change the situation either.
>
> The commit log mentioned:
>
>   "Even with "const" variables, the compiler will generate warnings about
>    VLA usage."
>
> So I guess the compiler (gcc?) is just not smart enough in this case?

Correct. This is technically a false positive, but with the goal of
adding -Wvla to the build globally, we have to get rid of these as
well. It's a little frustrating, I agree, but with all others fixed
now, these stand out. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH wireless-drivers] mt76x0: Remove VLA usage

[Stanislaw Gruszka]

On Wed, Aug 08, 2018 at 08:41:28AM -0700, Kees Cook wrote:
> >> I thought SPDX line is needed only if file has no license and eeprom.c
> >> file and other mt76x0 files have specified the license. Is SPDX still
> >> needed in that case ?
> 
> I thought all source files needed SPDX: https://lwn.net/Articles/739183/

Ok, goal is to have all kernel source files with SPDX header.

> >>> +#define MT_MAP_READS        DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)
> >>>  static int
> >>>  mt76x0_efuse_physical_size_check(struct mt76x0_dev *dev)
> >>>  {
> >>> -    const int map_reads = DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16);
> >>> -    u8 data[map_reads * 16];
> >>
> >> Why this is variable length array? DIV_ROUND_UP can not be calculated
> >> at compile time? But if so, macro do not change the situation either.
> >
> > The commit log mentioned:
> >
> >   "Even with "const" variables, the compiler will generate warnings about
> >    VLA usage."
> >
> > So I guess the compiler (gcc?) is just not smart enough in this case?
> 
> Correct. This is technically a false positive, but with the goal of
> adding -Wvla to the build globally, we have to get rid of these as
> well. It's a little frustrating, I agree, but with all others fixed
> now, these stand out. :)

Then:
Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

Re: [PATCH wireless-drivers] mt76x0: Remove VLA usage

[Kalle Valo]

Stanislaw Gruszka <sgruszka@redhat.com> writes:

> On Wed, Aug 08, 2018 at 08:41:28AM -0700, Kees Cook wrote:
>> >> I thought SPDX line is needed only if file has no license and eeprom.c
>> >> file and other mt76x0 files have specified the license. Is SPDX still
>> >> needed in that case ?
>> 
>> I thought all source files needed SPDX: https://lwn.net/Articles/739183/
>
> Ok, goal is to have all kernel source files with SPDX header.

Yeah, it's a goal but I don't think it's a hard requirement yet. At
least I don't see it as a reason to reject patches.

And besides, mt76 uses ISC license and that's not in LICENSES directory.
So someone should add that first, and I think that patch should go via
Jonathan Corbet's tree (CCed).

-- 
Kalle Valo

Re: [PATCH wireless-drivers] mt76x0: Remove VLA usage

[Kalle Valo]

Kees Cook <keescook@chromium.org> wrote:

> Even with "const" variables, the compiler will generate warnings about
> VLA usage. In the quest to remove all VLAs from the kernel[1], this uses
> a #define instead of a const to do the array sizing.
> 
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> 
> Fixes: e87b5039511a ("mt76x0: eeprom files")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

Patch applied to wireless-drivers-next.git, thanks.

17ad18fd12a3 mt76x0: Remove VLA usage

-- 
https://patchwork.kernel.org/patch/10559297/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches