Re: KASAN: use-after-free Read in usbhid_power

[Oliver Neukum <oneukum@suse.com>]

Am Mittwoch, den 24.07.2019, 17:02 -0400 schrieb Alan Stern:
> On Wed, 24 Jul 2019, Oliver Neukum wrote:
> 
> >  drivers/hid/usbhid/hid-core.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index c7bc9db5b192..98b996ecf4d3 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -1229,6 +1229,17 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> >  	struct usbhid_device *usbhid = hid->driver_data;
> >  	int r = 0;
> >  
> > +	spin_lock_irq(&usbhid->lock);
> > +	if (test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
> > +		r = -ENODEV;
> > +		spin_unlock_irq(&usbhid->lock);
> > +		goto bail_out;
> > +	} else {
> > +		/* protect against disconnect */
> > +		usb_get_dev(interface_to_usbdev(usbhid->intf));
> > +		spin_unlock_irq(&usbhid->lock);
> > +	}
> > +
> >  	switch (lvl) {
> >  	case PM_HINT_FULLON:
> >  		r = usb_autopm_get_interface(usbhid->intf);
> > @@ -1238,7 +1249,9 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> >  		usb_autopm_put_interface(usbhid->intf);
> >  		break;
> >  	}
> > +	usb_put_dev(interface_to_usbdev(usbhid->intf));
> >  
> > +bail_out:
> >  	return r;
> >  }
> 
> Isn't this treating the symptom instead of the cause?

Sort of. Holding a reference for the whole time would have merit,
but I doubt it is strictly necessary.

> Shouldn't the hid_device hold a reference to usbhid->intf throughout 
> its lifetime?  That way this sort of problem wouldn't arise in any 
> routine, not just usbhid_power().

Unfortunately the semantics would still be wrong without the check
in corner cases. In case disconnect() is called without a physical
unplug, we must not touch the power state.
I am indeed afraid that in that case my putative fix is still racy.
But I don't to just introduce a mutex just for this. Any ideas?

	Regards
		Oliver