Messages in this thread | | | Date | Sat, 23 Apr 2022 06:52:49 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH v0] nfc: nci: add flush_workqueue to prevent uaf |
| |
On Mon, Apr 18, 2022 at 09:59:10PM +0800, Lin Ma wrote: > Hello Guenter, > > > I have been wondering about this and the same code further below. > > What prevents the command timer from firing after the call to > > flush_workqueue() ? > > > > Thanks, > > Guenter > > > > From my understanding, once the flush_workqueue() is executed, the work that queued in > ndev->cmd_wq will be taken the care of. > > That is, once the flush_workqueue() is finished, it promises there is no executing or > pending nci_cmd_work() ever. > > static void nci_cmd_work(struct work_struct *work) > { > // ... > mod_timer(&ndev->cmd_timer, > jiffies + msecs_to_jiffies(NCI_CMD_TIMEOUT)); > // ... > } > > The command timer is still able be fired because the mod_timer() here. That is why the > del_timer_sync() is necessary after the flush_workqueue(). > > One very puzzling part is that you may find out the timer queue the work again > > /* NCI command timer function */ > static void nci_cmd_timer(struct timer_list *t) > { > // ... > queue_work(ndev->cmd_wq, &ndev->cmd_work); > } > > But I found that this is okay because there is no packets in ndev->cmd_q buffers hence > even there is a queued nci_cmd_work(), it simply checks the queue and returns. > > That is, the old race picture as below > > > Thread-1 Thread-2 > > | nci_dev_up() > > | nci_open_device() > > | __nci_request(nci_reset_req) > > | nci_send_cmd > > | queue_work(cmd_work) > > nci_unregister_device() | > > nci_close_device() | ... > > del_timer_sync(cmd_timer)[1] | > > ... | Worker > > nci_free_device() | nci_cmd_work() > > kfree(ndev)[3] | mod_timer(cmd_timer)[2] > > is impossible now because the patched flush_workqueue() make the race like below > > > Thread-1 Thread-2 > > | nci_dev_up() > > | nci_open_device() > > | __nci_request(nci_reset_req) > > | nci_send_cmd > > | queue_work(cmd_work) > > nci_unregister_device() | > > nci_close_device() | ... > > flush_workqueue()[patch] | Worker > > | nci_cmd_work() > > | mod_timer(cmd_timer)[2] > > // work over then return > > del_timer_sync(cmd_timer)[1] | > > | Timer > > | nci_cmd_timer() > > | > > // timer over then return | > > ... | > > nci_free_device() | > > kfree(ndev)[3] | > > > With above thinkings and the given fact that my POC didn't raise the UAF, I think the > flush_workqueue() + del_timer_sync() combination is okay to hinder this race. > > Tell me if there is anything wrong. >
Thanks a lot for the detailed explanation and analysis. I agree with your conclusion.
Guenter
| |