wifi: brcmfmac: fweh: Fix boot crash on Raspberry Pi 4

PATCH In review Gustavo A. R. Silva 4 participants 5 messages Feb 16, 2024 – Feb 27, 2024 Broadcom BRCM80211 IEEE802.11 wireless drivers

lore

Share & tools

lore

Gustavo A. R. Silva

author

2024-02-16 19:27 UTC #0 lore raw

PATCH 1/1 wifi: brcmfmac: fweh: Fix boot crash on Raspberry Pi 4

Fix boot crash on Raspberry Pi by moving the update to event->datalen before data is copied into flexible-array member data via memcpy().

Flexible-array member data was annotated with __counted_by(datalen) in commit 62d19b358088 ("wifi: brcmfmac: fweh: Add __counted_by for struct brcmf_fweh_queue_item and use struct_size()"). The intention of this is to gain visibility into the size of data at run-time through its _counter_ (in this case datalen), and with this have its accesses bounds-checked at run-time via CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS.

To effectively accomplish the above, we shall update the counter (datalen), before the first access to the flexible array (data), which was also done in the mentioned commit.

However, commit edec42821911 ("wifi: brcmfmac: allow per-vendor event handling") inadvertently caused a buffer overflow, detected by FORTIFY_SOURCE. It moved the event->datalen = datalen; update to after the first data access, at which point event->datalen was not yet updated from zero (after calling kzalloc()), leading to the overflow issue.

This fix repositions the event->datalen = datalen; update before accessing data, restoring the intended buffer overflow protection. :)

Fixes edec42821911 ("wifi: brcmfmac: allow per-vendor event handling")

Reported-by Nathan Chancellor <[email protected]>

Closes https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6

Tested-by Nathan Chancellor <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c +1 -1

index 0774f6c59226..f0b6a7607f16 100644
@@ -497,12 +497,12 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
 		return;
 
 	event->code = fwevt_idx;
+	event->datalen = datalen;
 	event->ifidx = event_packet->msg.ifidx;
 
 	/* use memcpy to get aligned event message */
 	memcpy(&event->emsg, &event_packet->msg, sizeof(event->emsg));
 	memcpy(event->data, data, datalen);
-	event->datalen = datalen;
 	memcpy(event->ifaddr, event_packet->eth.h_dest, ETH_ALEN);
 
 	brcmf_fweh_queue_event(fweh, event);

Signature · 1 line

2.34.1

Kees Cook

Reviewed-by on v1 1/1

2024-02-16 21:42 UTC #1 lore raw

Quoted 22 lines · Gustavo A. R. Silva

Fix boot crash on Raspberry Pi by moving the update to event->datalen before data is copied into flexible-array member data via memcpy().
Flexible-array member data was annotated with __counted_by(datalen) in commit 62d19b358088 ("wifi: brcmfmac: fweh: Add __counted_by for struct brcmf_fweh_queue_item and use struct_size()"). The intention of this is to gain visibility into the size of data at run-time through its _counter_ (in this case datalen), and with this have its accesses bounds-checked at run-time via CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS.
To effectively accomplish the above, we shall update the counter (datalen), before the first access to the flexible array (data), which was also done in the mentioned commit.
However, commit edec42821911 ("wifi: brcmfmac: allow per-vendor event handling") inadvertently caused a buffer overflow, detected by FORTIFY_SOURCE. It moved the event->datalen = datalen; update to after the first data access, at which point event->datalen was not yet updated from zero (after calling kzalloc()), leading to the overflow issue.

Quoted 8 lines

This fix repositions the event->datalen = datalen; update before accessing data, restoring the intended buffer overflow protection. :)
Fixes edec42821911 ("wifi: brcmfmac: allow per-vendor event handling")
Reported-by Nathan Chancellor <[email protected]>
Closes https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6
Tested-by Nathan Chancellor <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

Yup, this looks correct. Thanks!

Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

Kalle Valo

2024-02-27 09:19 UTC #2 lore raw

In reply to Gustavo A. R. Silva

Quoted 25 lines · Gustavo A. R. Silva

Fix boot crash on Raspberry Pi by moving the update to event->datalen before data is copied into flexible-array member data via memcpy().
Flexible-array member data was annotated with __counted_by(datalen) in commit 62d19b358088 ("wifi: brcmfmac: fweh: Add __counted_by for struct brcmf_fweh_queue_item and use struct_size()"). The intention of this is to gain visibility into the size of data at run-time through its _counter_ (in this case datalen), and with this have its accesses bounds-checked at run-time via CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS.
To effectively accomplish the above, we shall update the counter (datalen), before the first access to the flexible array (data), which was also done in the mentioned commit.
However, commit edec42821911 ("wifi: brcmfmac: allow per-vendor event handling") inadvertently caused a buffer overflow, detected by FORTIFY_SOURCE. It moved the event->datalen = datalen; update to after the first data access, at which point event->datalen was not yet updated from zero (after calling kzalloc()), leading to the overflow issue.
This fix repositions the event->datalen = datalen; update before accessing data, restoring the intended buffer overflow protection. :)

Quoted 6 lines

Fixes edec42821911 ("wifi: brcmfmac: allow per-vendor event handling")
Reported-by Nathan Chancellor <[email protected]>
Closes https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6
Tested-by Nathan Chancellor <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Kees Cook <[email protected]>

Arend, ack?

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/patch/Zc+3PFCUvLoVlpg8@neat/

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

Arend van Spriel

maintainer Acked-by

2024-02-27 11:12 UTC #3 lore raw

Quoted 27 lines · Kalle Valo

Quoted 25 lines · Gustavo A. R. Silva
Fix boot crash on Raspberry Pi by moving the update to event->datalen before data is copied into flexible-array member data via memcpy().
Flexible-array member data was annotated with __counted_by(datalen) in commit 62d19b358088 ("wifi: brcmfmac: fweh: Add __counted_by for struct brcmf_fweh_queue_item and use struct_size()"). The intention of this is to gain visibility into the size of data at run-time through its _counter_ (in this case datalen), and with this have its accesses bounds-checked at run-time via CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS.
To effectively accomplish the above, we shall update the counter (datalen), before the first access to the flexible array (data), which was also done in the mentioned commit.
However, commit edec42821911 ("wifi: brcmfmac: allow per-vendor event handling") inadvertently caused a buffer overflow, detected by FORTIFY_SOURCE. It moved the event->datalen = datalen; update to after the first data access, at which point event->datalen was not yet updated from zero (after calling kzalloc()), leading to the overflow issue.
This fix repositions the event->datalen = datalen; update before accessing data, restoring the intended buffer overflow protection. :)

Quoted 8 lines

Quoted 6 lines
Fixes edec42821911 ("wifi: brcmfmac: allow per-vendor event handling")
Reported-by Nathan Chancellor <[email protected]>
Closes https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6
Tested-by Nathan Chancellor <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Kees Cook <[email protected]>
Arend, ack?

Figured Kees Cook was the trumping authority here, but here it is:

Acked-by Arend van Spriel <[email protected]>

Gr. AvS

Kalle Valo

2024-02-27 14:43 UTC #4 lore raw

In reply to Gustavo A. R. Silva

Quoted 25 lines · Gustavo A. R. Silva

Fix boot crash on Raspberry Pi by moving the update to event->datalen before data is copied into flexible-array member data via memcpy().
Flexible-array member data was annotated with __counted_by(datalen) in commit 62d19b358088 ("wifi: brcmfmac: fweh: Add __counted_by for struct brcmf_fweh_queue_item and use struct_size()"). The intention of this is to gain visibility into the size of data at run-time through its _counter_ (in this case datalen), and with this have its accesses bounds-checked at run-time via CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS.
To effectively accomplish the above, we shall update the counter (datalen), before the first access to the flexible array (data), which was also done in the mentioned commit.
However, commit edec42821911 ("wifi: brcmfmac: allow per-vendor event handling") inadvertently caused a buffer overflow, detected by FORTIFY_SOURCE. It moved the event->datalen = datalen; update to after the first data access, at which point event->datalen was not yet updated from zero (after calling kzalloc()), leading to the overflow issue.
This fix repositions the event->datalen = datalen; update before accessing data, restoring the intended buffer overflow protection. :)

Quoted 7 lines

Fixes edec42821911 ("wifi: brcmfmac: allow per-vendor event handling")
Reported-by Nathan Chancellor <[email protected]>
Closes https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6
Tested-by Nathan Chancellor <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Kees Cook <[email protected]>
Acked-by Arend van Spriel <[email protected]>

Patch applied to wireless-next.git, thanks.

ec1aae190c77 wifi: brcmfmac: fweh: Fix boot crash on Raspberry Pi 4

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/patch/Zc+3PFCUvLoVlpg8@neat/

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