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: <524c4fb6.6e33.1803cf85ae9.Coremail.linma@zju.edu.cn> Date: Mon, 18 Apr 2022 21:59:10 +0800 (GMT+08:00) From: "Lin Ma" <linma@....edu.cn> To: "Guenter Roeck" <linux@...ck-us.net> Cc: krzk@...nel.org, davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, mudongliangabcd@...il.com Subject: Re: [PATCH v0] nfc: nci: add flush_workqueue to prevent uaf 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. Regards Lin Ma
Powered by blists - more mailing lists