[PATCH] nfc: port100: drain cmd_complete_work before freeing the device on disconnect

[PATCH] nfc: port100: drain cmd_complete_work before freeing the device on disconnect

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

port100_disconnect() tears the device down in an order that lets
cmd_complete_work run against freed memory:

	nfc_digital_unregister_device(dev->nfc_digital_dev);
	nfc_digital_free_device(dev->nfc_digital_dev);   /* ddev freed */

	usb_kill_urb(dev->in_urb);                       /* -> schedule_work() */
	usb_kill_urb(dev->out_urb);

	usb_free_urb(dev->in_urb);
	usb_free_urb(dev->out_urb);

	kfree(dev->cmd);                                 /* cmd freed */

nfc_digital_unregister_device() does not abort a transport command that
is already in flight, so an IN URB can still be pending here. The URB
completion handlers port100_recv_response() / port100_recv_ack() schedule
cmd_complete_work whenever the IN URB completes with an error status,
including the -ECONNRESET that usb_kill_urb(dev->in_urb) produces
synchronously for the in-flight command. The work handler
port100_wq_cmd_complete() then runs port100_send_async_complete():

	static void port100_send_async_complete(struct port100 *dev)
	{
		struct port100_cmd *cmd = dev->cmd;
		int status = cmd->status;		/* UAF read   */
		...
		cmd->complete_cb(dev->nfc_digital_dev, ...);	/* freed ddev */
		...
		kfree(cmd);				/* double free */
	}

Nothing drains the work, so a command in flight at disconnect leaves
cmd_complete_work queued (or running) and it dereferences dev->cmd (freed
by kfree(dev->cmd)), dev->nfc_digital_dev (freed by
nfc_digital_free_device()) and ultimately dev itself (devres-freed after
disconnect returns) -> a use-after-free plus double-free.

Kill the URBs and cancel_work_sync() the work *before*
nfc_digital_free_device(), so the completion that usb_kill_urb() forces
cannot re-queue the work and the handler cannot run (or is waited out)
while dev->nfc_digital_dev and dev are still valid. Placing the drain
after nfc_digital_free_device() would be insufficient: on SMP/PREEMPT a
worker that already started executing would be waited on by
cancel_work_sync() while dereferencing the already-freed ddev.

Found by code review and reproduced on 7.3-rc3 with KASAN and
DEBUG_OBJECTS, using dummy_hcd + raw_gadget to emulate the device and
race an in-flight command against USB disconnect: debugobjects reports a
free-active work_struct for port100_wq_cmd_complete, freed from
devres_release_all() via kfree() while still queued.

Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/nfc/port100.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/port100.c b/drivers/nfc/port100.c
--- a/drivers/nfc/port100.c
+++ b/drivers/nfc/port100.c
@@ -1619,13 +1619,15 @@ static void port100_disconnect(struct usb_interface *interface)
 	dev = usb_get_intfdata(interface);
 	usb_set_intfdata(interface, NULL);

 	nfc_digital_unregister_device(dev->nfc_digital_dev);
-	nfc_digital_free_device(dev->nfc_digital_dev);

 	usb_kill_urb(dev->in_urb);
 	usb_kill_urb(dev->out_urb);
+	cancel_work_sync(&dev->cmd_complete_work);
+
+	nfc_digital_free_device(dev->nfc_digital_dev);

 	usb_free_urb(dev->in_urb);
 	usb_free_urb(dev->out_urb);

 	kfree(dev->cmd);