Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Sat, 23 Apr 2022 06:52:49 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Lin Ma <linma@zju.edu.cn>
Cc:     krzk@kernel.org, davem@davemloft.net, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, mudongliangabcd@gmail.com
Subject: Re: [PATCH v0] nfc: nci: add flush_workqueue to prevent uaf
Message-ID: <20220423135249.GA3958174@roeck-us.net>
References: <20220412160430.11581-1-linma@zju.edu.cn>
 <20220418134133.GA872670@roeck-us.net>
 <524c4fb6.6e33.1803cf85ae9.Coremail.linma@zju.edu.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <524c4fb6.6e33.1803cf85ae9.Coremail.linma@zju.edu.cn>
Precedence: bulk

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