linux-hardening - Re: [PATCH][next] wifi: mt76: Replace zero-length array with flexible-array member

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 11 Apr 2023 12:39:06 -0600
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Simon Horman <simon.horman@...igine.com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     Felix Fietkau <nbd@....name>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Ryder Lee <ryder.lee@...iatek.com>,
        Shayne Chen <shayne.chen@...iatek.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] wifi: mt76: Replace zero-length array with
 flexible-array member



On 4/7/23 01:23, Simon Horman wrote:
> On Thu, Apr 06, 2023 at 08:32:12AM -0600, Gustavo A. R. Silva wrote:
>> Zero-length arrays are deprecated [1] and have to be replaced by C99
>> flexible-array members.
>>
>> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
>> on memcpy() and help to make progress towards globally enabling
>> -fstrict-flex-arrays=3 [2]
>>
>> Link: https://github.com/KSPP/linux/issues/78 [1]
>> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [2]
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> 
> Reviewed-by: Simon Horman <simon.horman@...igine.com>

Thanks :)

> 
>> ---
>>   drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h
>> index a5e6ee4daf92..9bf4b4199ee3 100644
>> --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h
>> +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.h
>> @@ -127,7 +127,7 @@ struct mt76_connac2_mcu_rxd {
>>   	u8 rsv1[2];
>>   	u8 s2d_index;
>>   
>> -	u8 tlv[0];
>> +	u8 tlv[];
>>   };
>>   
>>   struct mt76_connac2_patch_hdr {
> 
> Curiously -fstrict-flex-arrays=3 didn't flag this one in my environment,
> Ubuntu Lunar.

Yep; that's why I didn't include any warning message in the changelog text
this time.

And the reason for that is that tlv is not being indexed anywhere in the
code. However, it's being used in the pointer arithmetic operation below:

drivers/net/wireless/mediatek/mt76/mt7921/mcu.c:
  164         rxd = (struct mt76_connac2_mcu_rxd *)skb->data;
  165         grant = (struct mt7921_roc_grant_tlv *)(rxd->tlv + 4);


which means that it can be considered as an array of size greater than zero
at some point. Hence, it should be transformed into a C99 flexible array.

> 
>   gcc-13 --version
>   gcc-13 (Ubuntu 13-20230320-1ubuntu1) 13.0.1 20230320 (experimental) [master r13-6759-g5194ad1958c]
>   Copyright (C) 2023 Free Software Foundation, Inc.
>   This is free software; see the source for copying conditions.  There is NO
>   warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> I did, however, notice some other problems reported by gcc-13 with
> -fstrict-flex-arrays=3 in drivers/net/wireless/mediatek/mt76
> when run against net-next wireless. I've listed them in diff format below.
> 
> Are these on your radar too?

Yep; those are being addressed here:

https://lore.kernel.org/linux-hardening/ZBTUB%2FkJYQxq%2F6Cj@work/

--
Gustavo

> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/acpi_sar.h b/drivers/net/wireless/mediatek/mt76/mt7921/acpi_sar.h
> index 35268b0890ad..d09bb4eed1ec 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/acpi_sar.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/acpi_sar.h
> @@ -24,7 +24,7 @@ struct mt7921_asar_dyn {
>   	u8 names[4];
>   	u8 enable;
>   	u8 nr_tbl;
> -	struct mt7921_asar_dyn_limit tbl[0];
> +	struct mt7921_asar_dyn_limit tbl[];
>   } __packed;
>   
>   struct mt7921_asar_dyn_limit_v2 {
> @@ -37,7 +37,7 @@ struct mt7921_asar_dyn_v2 {
>   	u8 enable;
>   	u8 rsvd;
>   	u8 nr_tbl;
> -	struct mt7921_asar_dyn_limit_v2 tbl[0];
> +	struct mt7921_asar_dyn_limit_v2 tbl[];
>   } __packed;
>   
>   struct mt7921_asar_geo_band {
> @@ -55,7 +55,7 @@ struct mt7921_asar_geo {
>   	u8 names[4];
>   	u8 version;
>   	u8 nr_tbl;
> -	struct mt7921_asar_geo_limit tbl[0];
> +	struct mt7921_asar_geo_limit tbl[];
>   } __packed;
>   
>   struct mt7921_asar_geo_limit_v2 {
> @@ -69,7 +69,7 @@ struct mt7921_asar_geo_v2 {
>   	u8 version;
>   	u8 rsvd;
>   	u8 nr_tbl;
> -	struct mt7921_asar_geo_limit_v2 tbl[0];
> +	struct mt7921_asar_geo_limit_v2 tbl[];
>   } __packed;
>   
>   struct mt7921_asar_cl {
> @@ -85,7 +85,7 @@ struct mt7921_asar_fg {
>   	u8 rsvd;
>   	u8 nr_flag;
>   	u8 rsvd1;
> -	u8 flag[0];
> +	u8 flag[];
>   } __packed;
>   
>   struct mt7921_acpi_sar {