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

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

[Bui Quang Minh]

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.

Thank you,
Quang Minh.

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

[Dmitry Torokhov]

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