Messages in this thread | | | Date | Mon, 2 May 2022 08:34:07 +0200 | Subject | Re: [PATCH net v5 2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs | From | Krzysztof Kozlowski <> |
| |
On 29/04/2022 11:13, duoming@zju.edu.cn wrote: > 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.
fw_download_in_progress is not synchronized in nfcmrvl_nci_unregister_dev(), so even if fw_dnld_timeout() set it to true, the nfcmrvl_nci_unregister_dev() happening concurrently will not see updated fw_download_in_progress.
> > 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.
I am sorry, but you cannot move code around hoping it will by itself solve synchronization issues.
Best regards, Krzysztof
| |