| Message ID | Zc+3PFCUvLoVlpg8@neat (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ec1aae190c7729ffdd3603de311dc00f7ff988f9 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [next] wifi: brcmfmac: fweh: Fix boot crash on Raspberry Pi 4 | expand |
On Fri, Feb 16, 2024 at 01:27:56PM -0600, Gustavo A. R. Silva wrote: > 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 <nathan@kernel.org> > Closes: https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6 > Tested-by: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Yup, this looks correct. Thanks! Reviewed-by: Kees Cook <keescook@chromium.org>
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote: > 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 <nathan@kernel.org> > Closes: https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6 > Tested-by: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Reviewed-by: Kees Cook <keescook@chromium.org> Arend, ack?
On 2/27/2024 10:19 AM, Kalle Valo wrote: > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote: > >> 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 <nathan@kernel.org> >> Closes: https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6 >> Tested-by: Nathan Chancellor <nathan@kernel.org> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> Reviewed-by: Kees Cook <keescook@chromium.org> > > Arend, ack? Figured Kees Cook was the trumping authority here, but here it is: Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> Gr. AvS
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote: > 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 <nathan@kernel.org> > Closes: https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6 > Tested-by: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> Patch applied to wireless-next.git, thanks. ec1aae190c77 wifi: brcmfmac: fweh: Fix boot crash on Raspberry Pi 4
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c index 0774f6c59226..f0b6a7607f16 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c @@ -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);
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 <nathan@kernel.org> Closes: https://gist.github.com/nathanchance/e22f681f3bfc467f15cdf6605021aaa6 Tested-by: Nathan Chancellor <nathan@kernel.org> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)