linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
@ 2023-09-13  6:54 Juerg Haefliger
  2023-09-13  8:58 ` Kalle Valo
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Juerg Haefliger @ 2023-09-13  6:54 UTC (permalink / raw)
  To: aspriel, franky.lin, hante.meuleman, kvalo, linux-wireless,
	brcm80211-dev-list.pdl, SHA-cyfmac-dev-list
  Cc: linux-kernel, linus.walleij, marcan, keescook, gustavoars,
	hdegoede, juerg.haefliger, ryohei.kondo

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 */
 };
 
 /*
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-13  6:54 [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays Juerg Haefliger
@ 2023-09-13  8:58 ` Kalle Valo
  2023-09-13  9:17   ` Juerg Haefliger
  2023-09-13  9:28 ` Linus Walleij
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Kalle Valo @ 2023-09-13  8:58 UTC (permalink / raw)
  To: Juerg Haefliger
  Cc: aspriel, franky.lin, hante.meuleman, linux-wireless,
	brcm80211-dev-list.pdl, SHA-cyfmac-dev-list, linux-kernel,
	linus.walleij, marcan, keescook, gustavoars, hdegoede,
	ryohei.kondo

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-13  8:58 ` Kalle Valo
@ 2023-09-13  9:17   ` Juerg Haefliger
  2023-09-13 11:01     ` Kalle Valo
  0 siblings, 1 reply; 11+ messages in thread
From: Juerg Haefliger @ 2023-09-13  9:17 UTC (permalink / raw)
  To: Kalle Valo
  Cc: aspriel, franky.lin, hante.meuleman, linux-wireless,
	brcm80211-dev-list.pdl, SHA-cyfmac-dev-list, linux-kernel,
	linus.walleij, marcan, keescook, gustavoars, hdegoede,
	ryohei.kondo

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-13  6:54 [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays Juerg Haefliger
  2023-09-13  8:58 ` Kalle Valo
@ 2023-09-13  9:28 ` Linus Walleij
  2023-09-13 16:02 ` Gustavo A. R. Silva
  2023-09-14  7:02 ` [PATCH v2] " Juerg Haefliger
  3 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2023-09-13  9:28 UTC (permalink / raw)
  To: Juerg Haefliger
  Cc: aspriel, franky.lin, hante.meuleman, kvalo, linux-wireless,
	brcm80211-dev-list.pdl, SHA-cyfmac-dev-list, linux-kernel,
	marcan, keescook, gustavoars, hdegoede, ryohei.kondo

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-13  9:17   ` Juerg Haefliger
@ 2023-09-13 11:01     ` Kalle Valo
  0 siblings, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2023-09-13 11:01 UTC (permalink / raw)
  To: Juerg Haefliger
  Cc: aspriel, franky.lin, hante.meuleman, linux-wireless,
	brcm80211-dev-list.pdl, SHA-cyfmac-dev-list, linux-kernel,
	linus.walleij, marcan, keescook, gustavoars, hdegoede,
	ryohei.kondo

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-13  6:54 [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays Juerg Haefliger
  2023-09-13  8:58 ` Kalle Valo
  2023-09-13  9:28 ` Linus Walleij
@ 2023-09-13 16:02 ` Gustavo A. R. Silva
  2023-09-14  6:53   ` Juerg Haefliger
  2023-09-14  7:02 ` [PATCH v2] " Juerg Haefliger
  3 siblings, 1 reply; 11+ messages in thread
From: Gustavo A. R. Silva @ 2023-09-13 16:02 UTC (permalink / raw)
  To: Juerg Haefliger, aspriel, franky.lin, hante.meuleman, kvalo,
	linux-wireless, brcm80211-dev-list.pdl, SHA-cyfmac-dev-list
  Cc: linux-kernel, linus.walleij, marcan, keescook, gustavoars,
	hdegoede, ryohei.kondo



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

>   };
>   
>   /*

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-13 16:02 ` Gustavo A. R. Silva
@ 2023-09-14  6:53   ` Juerg Haefliger
  0 siblings, 0 replies; 11+ messages in thread
From: Juerg Haefliger @ 2023-09-14  6:53 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: aspriel, franky.lin, hante.meuleman, kvalo, linux-wireless,
	brcm80211-dev-list.pdl, SHA-cyfmac-dev-list, linux-kernel,
	linus.walleij, marcan, keescook, gustavoars, hdegoede,
	ryohei.kondo

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

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
> 
> >   };
> >   
> >   /*  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-13  6:54 [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays Juerg Haefliger
                   ` (2 preceding siblings ...)
  2023-09-13 16:02 ` Gustavo A. R. Silva
@ 2023-09-14  7:02 ` Juerg Haefliger
  2023-09-14 16:52   ` Kees Cook
                     ` (2 more replies)
  3 siblings, 3 replies; 11+ messages in thread
From: Juerg Haefliger @ 2023-09-14  7:02 UTC (permalink / raw)
  To: juerg.haefliger
  Cc: SHA-cyfmac-dev-list, aspriel, brcm80211-dev-list.pdl, franky.lin,
	gustavoars, hante.meuleman, hdegoede, keescook, kvalo,
	linus.walleij, linux-kernel, linux-wireless, marcan,
	ryohei.kondo, stable

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);

Cc: stable@vger.kernel.org # 6.5+
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>

---
v2:
  - Use element[] instead of DFA() in brcmf_chanspec_list.
  - Add Cc: stable tag
---
 .../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..611d1a6aabb9 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 */
+	__le32  element[];	/* variable length uint32 list */
 };
 
 /*
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-14  7:02 ` [PATCH v2] " Juerg Haefliger
@ 2023-09-14 16:52   ` Kees Cook
  2023-09-14 17:19   ` Gustavo A. R. Silva
  2023-09-18 13:20   ` Kalle Valo
  2 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2023-09-14 16:52 UTC (permalink / raw)
  To: Juerg Haefliger
  Cc: SHA-cyfmac-dev-list, aspriel, brcm80211-dev-list.pdl, franky.lin,
	gustavoars, hante.meuleman, hdegoede, kvalo, linus.walleij,
	linux-kernel, linux-wireless, marcan, ryohei.kondo, stable

On Thu, Sep 14, 2023 at 09:02:27AM +0200, 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);
> 
> Cc: stable@vger.kernel.org # 6.5+
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-14  7:02 ` [PATCH v2] " Juerg Haefliger
  2023-09-14 16:52   ` Kees Cook
@ 2023-09-14 17:19   ` Gustavo A. R. Silva
  2023-09-18 13:20   ` Kalle Valo
  2 siblings, 0 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2023-09-14 17:19 UTC (permalink / raw)
  To: Juerg Haefliger
  Cc: SHA-cyfmac-dev-list, aspriel, brcm80211-dev-list.pdl, franky.lin,
	gustavoars, hante.meuleman, hdegoede, keescook, kvalo,
	linus.walleij, linux-kernel, linux-wireless, marcan,
	ryohei.kondo, stable, linux-hardening



On 9/14/23 01:02, 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);
> 
> Cc: stable@vger.kernel.org # 6.5+
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks
--
Gustavo

> 
> ---
> v2:
>    - Use element[] instead of DFA() in brcmf_chanspec_list.
>    - Add Cc: stable tag
> ---
>   .../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..611d1a6aabb9 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 */
> +	__le32  element[];	/* variable length uint32 list */
>   };
>   
>   /*

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
  2023-09-14  7:02 ` [PATCH v2] " Juerg Haefliger
  2023-09-14 16:52   ` Kees Cook
  2023-09-14 17:19   ` Gustavo A. R. Silva
@ 2023-09-18 13:20   ` Kalle Valo
  2 siblings, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2023-09-18 13:20 UTC (permalink / raw)
  To: Juerg Haefliger
  Cc: juerg.haefliger, SHA-cyfmac-dev-list, aspriel,
	brcm80211-dev-list.pdl, franky.lin, gustavoars, hante.meuleman,
	hdegoede, keescook, linus.walleij, linux-kernel, linux-wireless,
	marcan, ryohei.kondo, stable

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);
> 
> Cc: stable@vger.kernel.org # 6.5+
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Patch applied to wireless.git, thanks.

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230914070227.12028-1-juerg.haefliger@canonical.com/

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-09-18 17:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-13  6:54 [PATCH] wifi: brcmfmac: Replace 1-element arrays with flexible arrays Juerg Haefliger
2023-09-13  8:58 ` Kalle Valo
2023-09-13  9:17   ` Juerg Haefliger
2023-09-13 11:01     ` Kalle Valo
2023-09-13  9:28 ` Linus Walleij
2023-09-13 16:02 ` Gustavo A. R. Silva
2023-09-14  6:53   ` Juerg Haefliger
2023-09-14  7:02 ` [PATCH v2] " Juerg Haefliger
2023-09-14 16:52   ` Kees Cook
2023-09-14 17:19   ` Gustavo A. R. Silva
2023-09-18 13:20   ` Kalle Valo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).