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