Messages in this thread | | | Date | Tue, 27 Feb 2024 12:12:57 +0100 | Subject | Re: [PATCH][next] wifi: brcmfmac: fweh: Fix boot crash on Raspberry Pi 4 | From | Arend van Spriel <> |
| |
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 [unhandled content-type:application/pkcs7-signature] | |