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: <73fe1723.69fe.1807498ab4d.Coremail.duoming@zju.edu.cn> Date: Fri, 29 Apr 2022 17:13:24 +0800 (GMT+08:00) From: duoming@....edu.cn To: "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org> Cc: linux-kernel@...r.kernel.org, kuba@...nel.org, gregkh@...uxfoundation.org, 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: Re: [PATCH net v5 2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs Hello, On Fri, 29 Apr 2022 09:27:48 +0200 Krzysztof 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? I think it could be prevented. We use nci_unregister_device() to synchronize, if the firmware download routine is running, the cleanup routine will wait it to finish. The flag "fw_download_in_progress" will be set to false, if the the firmware download routine is finished. Although the timer handler fw_dnld_timeout() could be running, nfcmrvl_nci_unregister_dev() will check the flag "fw_download_in_progress" which is already set to false and nfcmrvl_fw_dnld_abort() in nfcmrvl_nci_unregister_dev() will not execute. > > > > 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. Thanks, I will strip unrelated parts of oops. > > > > 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... The firmware download process is shown below: nfc_genl_fw_download()->nfc_fw_download()->nci_fw_download()->nfcmrvl_nci_fw_download() ->nfcmrvl_fw_dnld_start() So T2 calls is a part of firmware download routine in nfc core layer. We use bool variable and device_lock() to synchronize between cleanup routine and firmware download routine in nfc core layer. The above picture is attempt to show this synchronized process. > > > > 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, Duoming Zhou
Powered by blists - more mailing lists