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
