* [BUG REPORT] usb: usb-skeleton: Race condition between skel_open and skel_disconnect
@ 2021-05-17 14:31 Bui Quang Minh
2021-05-24 18:50 ` Dmitry Torokhov
0 siblings, 1 reply; 2+ messages in thread
From: Bui Quang Minh @ 2021-05-17 14:31 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [BUG REPORT] usb: usb-skeleton: Race condition between skel_open and skel_disconnect
2021-05-17 14:31 [BUG REPORT] usb: usb-skeleton: Race condition between skel_open and skel_disconnect Bui Quang Minh
@ 2021-05-24 18:50 ` Dmitry Torokhov
0 siblings, 0 replies; 2+ messages in thread
From: Dmitry Torokhov @ 2021-05-24 18:50 UTC (permalink / raw)
To: Bui Quang Minh; +Cc: Greg Kroah-Hartman, linux-usb
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-05-24 18:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17 14:31 [BUG REPORT] usb: usb-skeleton: Race condition between skel_open and skel_disconnect Bui Quang Minh
2021-05-24 18:50 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).