Re: [BUG REPORT] usb: usb-skeleton: Race condition between skel_open and skel_disconnect

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi,

On Mon, May 17, 2021 at 09:31:57PM +0700, Bui Quang Minh wrote:
> Hi,
> 
> I spotted this bug through code review and I don't know how to make a Proof
> of Concept for this bug so maybe I'm wrong.
> 
> Between skel_open() and skel_disconnect(), this scenario can happen
> 
> skel_open()			skel_disconnect()
> dev = usb_get_intfdata(interface);
> 				usb_set_intfdata(interface, NULL);
> 				kref_put(&dev->kref, skel_delete);
> kref_get(&dev->kref);
> 
> In case dev's refcount is 1 before these events, kref_put() in
> skel_disconnect() will call the skel_delete to free dev. As a result, a UAF
> will happen when we try to access dev->kref in skel_open(). I can see this
> pattern in other USB drivers as well such as usblcd.c, yurex.c, ...
> 
> Please correct me if I am wrong.

I think it is mostly OK. As far as I can see the minor_rwsem in
drivers/usb/core/file.c makes sure that usb_open()/skel_open() either
complete if they happen before call to usb_deregister_dev() in
skel_disconnect() or usb_open() errors out and not call into skel_open()
if usb_deregister_dev() already completed. So there is no issue with
bumping refcount while the structure is being deleted.

This only leaves usb_get_intfdata() returning NULL because we set it to
NULL too early in skel_disconnect(). I'd recommend moving
"usb_set_intfdata(interface, NULL)" to happen after call to
usb_deregister_dev() in skel_disconnect().

Thanks.

-- 
Dmitry