Re: [PATCH v0] nfc: nci: add flush_workqueue to prevent uaf

["Lin Ma" <linma@zju.edu.cn>]

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