wifi: brcmfmac: Replace 1-element arrays with flexible arrays

  Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC"),
UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. Walking
'element' and 'channel_list' will trigger warnings, so make them proper
flexible arrays.

False positive warnings were:

  UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6984:20
  index 1 is out of range for type '__le32 [1]'

  UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1126:27
  index 1 is out of range for type '__le16 [1]'

for these lines of code:

  6884  ch.chspec = (u16)le32_to_cpu(list->element[i]);

  1126  params_le->channel_list[i] = cpu_to_le16(chanspec);

Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
---
 .../wireless/broadcom/brcm80211/brcmfmac/fwil_types.h    | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
  

On Wed, 13 Sep 2023 11:58:07 +0300
Kalle Valo <kvalo@kernel.org> wrote:

> Juerg Haefliger <juerg.haefliger@canonical.com> writes:
> 
> > Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC"),
> > UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. Walking
> > 'element' and 'channel_list' will trigger warnings, so make them proper
> > flexible arrays.
> >
> > False positive warnings were:
> >
> >   UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6984:20
> >   index 1 is out of range for type '__le32 [1]'
> >
> >   UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1126:27
> >   index 1 is out of range for type '__le16 [1]'
> >
> > for these lines of code:
> >
> >   6884  ch.chspec = (u16)le32_to_cpu(list->element[i]);
> >
> >   1126  params_le->channel_list[i] = cpu_to_le16(chanspec);
> >
> > Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>  
> 
> Should this be queued for v6.6?

I would think so. It's a problem since 6.5. Which reminds me that I should
have added:

Cc: stable@vger.kernel.org # 6.5+

...Juerg
  

On Wed, Sep 13, 2023 at 8:54 AM Juerg Haefliger
<juerg.haefliger@canonical.com> wrote:

> Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC"),
> UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. Walking
> 'element' and 'channel_list' will trigger warnings, so make them proper
> flexible arrays.
>
> False positive warnings were:
>
>   UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6984:20
>   index 1 is out of range for type '__le32 [1]'
>
>   UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1126:27
>   index 1 is out of range for type '__le16 [1]'
>
> for these lines of code:
>
>   6884  ch.chspec = (u16)le32_to_cpu(list->element[i]);
>
>   1126  params_le->channel_list[i] = cpu_to_le16(chanspec);
>
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>

Obviously the right solution, thanks for looking into this!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
  

Juerg Haefliger <juerg.haefliger@canonical.com> writes:

> On Wed, 13 Sep 2023 11:58:07 +0300
> Kalle Valo <kvalo@kernel.org> wrote:
>
>> Juerg Haefliger <juerg.haefliger@canonical.com> writes:
>> 
>> > Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC"),
>> > UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. Walking
>> > 'element' and 'channel_list' will trigger warnings, so make them proper
>> > flexible arrays.
>> >
>> > False positive warnings were:
>> >
>> >   UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6984:20
>> >   index 1 is out of range for type '__le32 [1]'
>> >
>> >   UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1126:27
>> >   index 1 is out of range for type '__le16 [1]'
>> >
>> > for these lines of code:
>> >
>> >   6884  ch.chspec = (u16)le32_to_cpu(list->element[i]);
>> >
>> >   1126  params_le->channel_list[i] = cpu_to_le16(chanspec);
>> >
>> > Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>  
>> 
>> Should this be queued for v6.6?
>
> I would think so. It's a problem since 6.5. Which reminds me that I should
> have added:
>
> Cc: stable@vger.kernel.org # 6.5+

I can add that during commit.
  

On 9/13/23 00:54, Juerg Haefliger wrote:
> Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC"),
> UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. Walking
> 'element' and 'channel_list' will trigger warnings, so make them proper
> flexible arrays.
> 
> False positive warnings were:
> 
>    UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6984:20
>    index 1 is out of range for type '__le32 [1]'
> 
>    UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1126:27
>    index 1 is out of range for type '__le16 [1]'
> 
> for these lines of code:
> 
>    6884  ch.chspec = (u16)le32_to_cpu(list->element[i]);
> 
>    1126  params_le->channel_list[i] = cpu_to_le16(chanspec);
> 
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/fwil_types.h    | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> index bece26741d3a..ed723a5b5d54 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> @@ -442,7 +442,12 @@ struct brcmf_scan_params_v2_le {
>   				 * fixed parameter portion is assumed, otherwise
>   				 * ssid in the fixed portion is ignored
>   				 */
> -	__le16 channel_list[1];	/* list of chanspecs */
> +	union {
> +		__le16 padding;	/* Reserve space for at least 1 entry for abort
> +				 * which uses an on stack brcmf_scan_params_v2_le
> +				 */
> +		DECLARE_FLEX_ARRAY(__le16, channel_list);	/* chanspecs */
> +	};
>   };
>   
>   struct brcmf_scan_results {
> @@ -702,7 +707,7 @@ struct brcmf_sta_info_le {
>   
>   struct brcmf_chanspec_list {
>   	__le32	count;		/* # of entries */
> -	__le32	element[1];	/* variable length uint32 list */
> +	DECLARE_FLEX_ARRAY(__le32, element);	/* variable length uint32 list */

If no padding is needed, as in the other case, then DFA() is not necessary.
Just remove the 1 from the array declaration:

  struct brcmf_chanspec_list {
         __le32  count;          /* # of entries */
-       __le32  element[1];     /* variable length uint32 list */
+       __le32  element[];      /* variable length uint32 list */
  };

--
Gustavo

>   };
>   
>   /*
  

On Wed, 13 Sep 2023 10:02:12 -0600
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:

> On 9/13/23 00:54, Juerg Haefliger wrote:
> > Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC"),
> > UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. Walking
> > 'element' and 'channel_list' will trigger warnings, so make them proper
> > flexible arrays.
> > 
> > False positive warnings were:
> > 
> >    UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6984:20
> >    index 1 is out of range for type '__le32 [1]'
> > 
> >    UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1126:27
> >    index 1 is out of range for type '__le16 [1]'
> > 
> > for these lines of code:
> > 
> >    6884  ch.chspec = (u16)le32_to_cpu(list->element[i]);
> > 
> >    1126  params_le->channel_list[i] = cpu_to_le16(chanspec);
> > 
> > Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> > ---
> >   .../wireless/broadcom/brcm80211/brcmfmac/fwil_types.h    | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> > index bece26741d3a..ed723a5b5d54 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> > @@ -442,7 +442,12 @@ struct brcmf_scan_params_v2_le {
> >   				 * fixed parameter portion is assumed, otherwise
> >   				 * ssid in the fixed portion is ignored
> >   				 */
> > -	__le16 channel_list[1];	/* list of chanspecs */
> > +	union {
> > +		__le16 padding;	/* Reserve space for at least 1 entry for abort
> > +				 * which uses an on stack brcmf_scan_params_v2_le
> > +				 */
> > +		DECLARE_FLEX_ARRAY(__le16, channel_list);	/* chanspecs */
> > +	};
> >   };
> >   
> >   struct brcmf_scan_results {
> > @@ -702,7 +707,7 @@ struct brcmf_sta_info_le {
> >   
> >   struct brcmf_chanspec_list {
> >   	__le32	count;		/* # of entries */
> > -	__le32	element[1];	/* variable length uint32 list */
> > +	DECLARE_FLEX_ARRAY(__le32, element);	/* variable length uint32 list */  
> 
> If no padding is needed, as in the other case, then DFA() is not necessary.
> Just remove the 1 from the array declaration:
> 
>   struct brcmf_chanspec_list {
>          __le32  count;          /* # of entries */
> -       __le32  element[1];     /* variable length uint32 list */
> +       __le32  element[];      /* variable length uint32 list */
>   };

Ah, I wasn't sure if that is still acceptable. Will send a v2.

...Juerg

 
> --
> Gustavo
> 
> >   };
> >   
> >   /*