lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <8656d527-94ab-228f-66f1-06e5d533e16a@linaro.org> Date: Fri, 29 Apr 2022 09:27:48 +0200 From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> To: Duoming Zhou <duoming@....edu.cn>, linux-kernel@...r.kernel.org, kuba@...nel.org, gregkh@...uxfoundation.org Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com, alexander.deucher@....com, akpm@...ux-foundation.org, broonie@...nel.org, netdev@...r.kernel.org, linma@....edu.cn Subject: Re: [PATCH net v5 2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs On 29/04/2022 03:14, Duoming Zhou wrote: > There are destructive operations such as nfcmrvl_fw_dnld_abort and > gpio_free in nfcmrvl_nci_unregister_dev. The resources such as firmware, > gpio and so on could be destructed while the upper layer functions such as > nfcmrvl_fw_dnld_start and nfcmrvl_nci_recv_frame is executing, which leads > to double-free, use-after-free and null-ptr-deref bugs. > > There are three situations that could lead to double-free bugs. > > The first situation is shown below: > > (Thread 1) | (Thread 2) > nfcmrvl_fw_dnld_start | > ... | nfcmrvl_nci_unregister_dev > release_firmware() | nfcmrvl_fw_dnld_abort > kfree(fw) //(1) | fw_dnld_over > | release_firmware > ... | kfree(fw) //(2) > | ... > > The second situation is shown below: > > (Thread 1) | (Thread 2) > nfcmrvl_fw_dnld_start | > ... | > mod_timer | > (wait a time) | > fw_dnld_timeout | nfcmrvl_nci_unregister_dev > fw_dnld_over | nfcmrvl_fw_dnld_abort > release_firmware | fw_dnld_over > kfree(fw) //(1) | release_firmware > ... | kfree(fw) //(2) How exactly the case here is being prevented? If nfcmrvl_nci_unregister_dev() happens slightly earlier, before fw_dnld_timeout() on the left side (T1), the T1 will still hit it, won't it? > > The third situation is shown below: > > (Thread 1) | (Thread 2) > nfcmrvl_nci_recv_frame | > if(..->fw_download_in_progress)| > nfcmrvl_fw_dnld_recv_frame | > queue_work | > | > fw_dnld_rx_work | nfcmrvl_nci_unregister_dev > fw_dnld_over | nfcmrvl_fw_dnld_abort > release_firmware | fw_dnld_over > kfree(fw) //(1) | release_firmware > | kfree(fw) //(2) > > The firmware struct is deallocated in position (1) and deallocated > in position (2) again. > > The crash trace triggered by POC is like below: > > [ 122.640457] BUG: KASAN: double-free or invalid-free in fw_dnld_over+0x28/0xf0 > [ 122.640457] Call Trace: > [ 122.640457] <TASK> > [ 122.640457] kfree+0xb0/0x330 > [ 122.640457] fw_dnld_over+0x28/0xf0 > [ 122.640457] nfcmrvl_nci_unregister_dev+0x61/0x70 > [ 122.640457] nci_uart_tty_close+0x87/0xd0 > [ 122.640457] tty_ldisc_kill+0x3e/0x80 > [ 122.640457] tty_ldisc_hangup+0x1b2/0x2c0 > [ 122.640457] __tty_hangup.part.0+0x316/0x520 > [ 122.640457] tty_release+0x200/0x670 > [ 122.640457] __fput+0x110/0x410 > [ 122.640457] task_work_run+0x86/0xd0 > [ 122.640457] exit_to_user_mode_prepare+0x1aa/0x1b0 > [ 122.640457] syscall_exit_to_user_mode+0x19/0x50 > [ 122.640457] do_syscall_64+0x48/0x90 > [ 122.640457] entry_SYSCALL_64_after_hwframe+0x44/0xae > [ 122.640457] RIP: 0033:0x7f68433f6beb Please always strip unrelated parts of oops, so minimum the timing. The addresses could be removed as well. > > What's more, there are also use-after-free and null-ptr-deref bugs > in nfcmrvl_fw_dnld_start. If we deallocate firmware struct, gpio or > set null to the members of priv->fw_dnld in nfcmrvl_nci_unregister_dev, > then, we dereference firmware, gpio or the members of priv->fw_dnld in > nfcmrvl_fw_dnld_start, the UAF or NPD bugs will happen. > > This patch reorders destructive operations after nci_unregister_device > and uses bool variable in nfc_dev to synchronize between cleanup routine > and firmware download routine. The process is shown below. > > (Thread 1) | (Thread 2) > nfcmrvl_nci_unregister_dev | > nci_unregister_device | > nfc_unregister_device | nfc_fw_download > device_lock() | > ... | > dev->dev_register = false; | ... > device_unlock() | > ... | device_lock() > (destructive operations) | if(!dev->dev_register) > | goto error; > | error: > | device_unlock() How T2 calls appeared here? They were not present in any of your previous process-examples... > > If the device is detaching, the download function will goto error. > If the download function is executing, nci_unregister_device will > wait until download function is finished. > > Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support") > Signed-off-by: Duoming Zhou <duoming@....edu.cn> > --- > Changes in v5: > - Use bool variable added in patch 1/2 to synchronize. > > drivers/nfc/nfcmrvl/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c > index 2fcf545012b..1a5284de434 100644 > --- a/drivers/nfc/nfcmrvl/main.c > +++ b/drivers/nfc/nfcmrvl/main.c > @@ -183,6 +183,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv) > { > struct nci_dev *ndev = priv->ndev; > > + nci_unregister_device(ndev); > if (priv->ndev->nfc_dev->fw_download_in_progress) > nfcmrvl_fw_dnld_abort(priv); > > @@ -191,7 +192,6 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv) > if (gpio_is_valid(priv->config.reset_n_io)) > gpio_free(priv->config.reset_n_io); > > - nci_unregister_device(ndev); > nci_free_device(ndev); > kfree(priv); > } Best regards, Krzysztof
Powered by blists - more mailing lists