Re: [PATCH] brcmfmac: Fix out of bounds memory access during fw load

[Kalle Valo]

Lyude Paul <lyude@xxxxxxxxxx> writes:

> I ended up tracking down some rather nasty issues with f2fs (and other
> filesystem modules) constantly crashing on my kernel down to a
> combination of out of bounds memory accesses, one of which was coming
> from brcmfmac during module load:
>
> [   30.891382] brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4356-sdio for chip BCM4356/2
> [   30.894437] ==================================================================
> [   30.901581] BUG: KASAN: global-out-of-bounds in brcmf_fw_alloc_request+0x42c/0x480 [brcmfmac]
> [   30.909935] Read of size 1 at addr ffff2000024865df by task kworker/6:2/387
> [   30.916805]
> [   30.918261] CPU: 6 PID: 387 Comm: kworker/6:2 Tainted: G           O      4.20.0-rc3Lyude-Test+ #19
> [   30.927251] Hardware name: amlogic khadas-vim2/khadas-vim2, BIOS 2018.07-rc2-armbian 09/11/2018
> [   30.935964] Workqueue: events brcmf_driver_register [brcmfmac]
> [   30.941641] Call trace:
> [   30.944058]  dump_backtrace+0x0/0x3e8
> [   30.947676]  show_stack+0x14/0x20
> [   30.950968]  dump_stack+0x130/0x1c4
> [   30.954406]  print_address_description+0x60/0x25c
> [   30.959066]  kasan_report+0x1b4/0x368
> [   30.962683]  __asan_report_load1_noabort+0x18/0x20
> [   30.967547]  brcmf_fw_alloc_request+0x42c/0x480 [brcmfmac]
> [   30.967639]  brcmf_sdio_probe+0x163c/0x2050 [brcmfmac]
> [   30.978035]  brcmf_ops_sdio_probe+0x598/0xa08 [brcmfmac]
> [   30.983254]  sdio_bus_probe+0x190/0x398
> [   30.983270]  really_probe+0x2a0/0xa70
> [   30.983296]  driver_probe_device+0x1b4/0x2d8
> [   30.994901]  __driver_attach+0x200/0x280
> [   30.994914]  bus_for_each_dev+0x10c/0x1a8
> [   30.994925]  driver_attach+0x38/0x50
> [   30.994935]  bus_add_driver+0x330/0x608
> [   30.994953]  driver_register+0x140/0x388
> [   31.013965]  sdio_register_driver+0x74/0xa0
> [   31.014076]  brcmf_sdio_register+0x14/0x60 [brcmfmac]
> [   31.023177]  brcmf_driver_register+0xc/0x18 [brcmfmac]
> [   31.023209]  process_one_work+0x654/0x1080
> [   31.032266]  worker_thread+0x4f0/0x1308
> [   31.032286]  kthread+0x2a8/0x320
> [   31.039254]  ret_from_fork+0x10/0x1c
> [   31.039269]
> [   31.044226] The buggy address belongs to the variable:
> [   31.044351]  brcmf_firmware_path+0x11f/0xfffffffffffd3b40 [brcmfmac]
> [   31.055601]
> [   31.057031] Memory state around the buggy address:
> [   31.061800]  ffff200002486480: 04 fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
> [   31.068983]  ffff200002486500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [   31.068993] >ffff200002486580: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
> [   31.068999]                                                     ^
> [   31.069017]  ffff200002486600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [   31.096521]  ffff200002486680: 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
> [   31.096528] ==================================================================
> [   31.096533] Disabling lock debugging due to kernel taint
>
> It appears that when trying to determine the length of the string in the
> alternate firmware path, we make the mistake of not handling the case
> where the firmware path is empty correctly. Since strlen(mp_path) can
> return 0, we'll end up accessing mp_path[-1] when the firmware_path
> isn't provided through the module arguments.
>
> So, fix this by just setting the end char to '\0' by default, and only
> changing it if we have a non-zero length. Additionally, use strnlen()
> with BRCMF_FW_ALTPATH_LEN instead of strlen() just to be extra safe.
>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
>
> Fixes: 2baa3aaee27f ("brcmfmac: introduce brcmf_fw_alloc_request() function")
> Cc: Hante Meuleman <hante.meuleman@xxxxxxxxxxxx>
> Cc: Pieter-Paul Giesberts <pieter-paul.giesberts@xxxxxxxxxxxx>
> Cc: Franky Lin <franky.lin@xxxxxxxxxxxx>
> Cc: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> Cc: Kalle Valo <kvalo@xxxxxxxxxxxxxx>
> Cc: Arend Van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> Cc: Himanshu Jha <himanshujha199640@xxxxxxxxx>
> Cc: Dan Haab <dhaab@xxxxxxxxx>
> Cc: Jia-Shyr Chuang <saint.chuang@xxxxxxxxxxx>
> Cc: Ian Molton <ian@xxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.17+
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>

Lyude's s-o-b is twice, I can fix that.

Arend, should I queue this to 4.20?

-- 
Kalle Valo