port100: use after free of dev->cmd if disconnect races cmd_complete_work

[Maoyi Xie]

Hi all,

I was looking at the port100 driver after the earlier port100_send_complete()
use after free fix, and I think port100_disconnect() can still leave a use after
free because it never cancels dev->cmd_complete_work. I am not completely sure,
so I would appreciate it if you could let me know whether you see this as a real
problem.

The command completion work is scheduled from the two URB completion handlers,
port100_recv_response() and port100_recv_ack(), and both of them call
schedule_work(&dev->cmd_complete_work). The worker, port100_send_async_complete(),
then dereferences dev->cmd.

  static void port100_send_async_complete(struct port100 *dev)
  {
	struct port100_cmd *cmd = dev->cmd;
	int status = cmd->status;
	...
	cmd->complete_cb(dev, cmd->complete_cb_context, resp);
	kfree(cmd);
  }

Here is what port100_disconnect() does.

  usb_kill_urb(dev->in_urb);
  usb_kill_urb(dev->out_urb);
  usb_free_urb(dev->in_urb);
  usb_free_urb(dev->out_urb);
  kfree(dev->cmd);

usb_kill_urb() waits for a completion handler that is already running to return,
but a cmd_complete_work that the handler already scheduled stays queued, and
nothing cancels it. dev is allocated with devm, so once disconnect() returns dev
is freed as well. The worker then runs port100_send_async_complete() and reads
dev->cmd and cmd->status out of freed memory.

I do not have a port100 adapter, so I reproduced just the ordering between the
work and the free on 7.1-rc7 under KASAN. A small harness sets up a struct
port100 with a dev->cmd, calls schedule_work(&dev->cmd_complete_work), then frees
dev->cmd the way disconnect() does and lets the worker run. KASAN reports this.

  BUG: KASAN: slab-use-after-free in port100_poc_work+0x7c/0x90
  Read of size 4 at addr ffff888106322004 by task kworker/1:2/77
  Workqueue: events port100_poc_work
  ...
  Freed by task 286:
   kfree+0x137/0x3c0
   ...                                  <- the kfree(dev->cmd) from disconnect
  The buggy address is located 4 bytes inside of freed 64-byte region

The read is cmd->status (offset 4 in struct port100_cmd), done by the worker
after dev->cmd was freed.

The fix I tried is to cancel the work after killing the URBs, so no completion
can schedule it again, and before freeing dev->cmd.

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

  +	cancel_work_sync(&dev->cmd_complete_work);
  +
  	usb_free_urb(dev->in_urb);
  	usb_free_urb(dev->out_urb);

  	kfree(dev->cmd);

With that change the same harness either runs the work on a command that is still
valid or cancels it, and the KASAN report is gone. The earlier fix (f80cfe2f2658)
only added usb_kill_urb() to the probe error path, so this disconnect path looks
unaddressed. It has been there since the async command mechanism went in
(0347a6ab300a), so it would be a stable candidate.

Would you agree this is a real use after free, and is cancel_work_sync() in
port100_disconnect() the right fix? I am happy to send a proper patch with the
reproducer once you confirm.

Thanks,
Maoyi
https://maoyixie.com/