| Message ID | 20211219060241.4125076-1-mudongliangabcd@gmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | driver: powermate: kill urb in the disconnect function | expand |
Hi Dongliang, On Sun, Dec 19, 2021 at 02:02:15PM +0800, Dongliang Mu wrote: > In powermate_disconnect, powermate_pulse_led will invoke > powermate_sync_state and submit one urb with pm as its context. > If powermate disconnect before the execution of complete handler, > the pm will become a dangling pointer and lead to UAF. > > Fix this by calling usb_kill_urb(pm->config) in the disconnect function. > Note that, the error handling error does not need to take care of this. Unfortunately this analysis is not correct and adding usb_kill_urb() does not really help, as while input device is registered there may be new events coming into the driver that may cause config URB be submitted again. What actually happens is that call to usb_free_urb(pm->config) below tries to properly shutdown and destroy URB, but unfortunately powermate_config_complete() handler only uses urb->status to print error message, and resubmits the URB that is about to be freed again. The URB completion handler needs to check for -ESHUTDOWN (-108) and other terminal error codes before attempting to resubmit the URB. > > Reported-by: syzbot+9780d2b05ac158d32284@syzkaller.appspotmail.com > Fixes: ba0acb5ee318901 ("Input: move USB miscellaneous devices under drivers/input/misc") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/input/misc/powermate.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c > index c4e0e1886061..903993469fde 100644 > --- a/drivers/input/misc/powermate.c > +++ b/drivers/input/misc/powermate.c > @@ -424,6 +424,7 @@ static void powermate_disconnect(struct usb_interface *intf) > if (pm) { > pm->requires_update = 0; > usb_kill_urb(pm->irq); > + usb_kill_urb(pm->config); > input_unregister_device(pm->input); > usb_free_urb(pm->irq); > usb_free_urb(pm->config); > -- > 2.25.1 > Thanks.
diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c index c4e0e1886061..903993469fde 100644 --- a/drivers/input/misc/powermate.c +++ b/drivers/input/misc/powermate.c @@ -424,6 +424,7 @@ static void powermate_disconnect(struct usb_interface *intf) if (pm) { pm->requires_update = 0; usb_kill_urb(pm->irq); + usb_kill_urb(pm->config); input_unregister_device(pm->input); usb_free_urb(pm->irq); usb_free_urb(pm->config);
In powermate_disconnect, powermate_pulse_led will invoke powermate_sync_state and submit one urb with pm as its context. If powermate disconnect before the execution of complete handler, the pm will become a dangling pointer and lead to UAF. Fix this by calling usb_kill_urb(pm->config) in the disconnect function. Note that, the error handling error does not need to take care of this. Reported-by: syzbot+9780d2b05ac158d32284@syzkaller.appspotmail.com Fixes: ba0acb5ee318901 ("Input: move USB miscellaneous devices under drivers/input/misc") Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/input/misc/powermate.c | 1 + 1 file changed, 1 insertion(+)