lkml.org 
[lkml]   [2022]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-04-23 15:56    [W:0.164 / U:0.584 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site