Messages in this thread Patch in this message | | | From | "Zhang, Qiang1" <> | Subject | RE: [syzbot] KASAN: use-after-free Read in dev_uevent | Date | Thu, 24 Feb 2022 03:14:54 +0000 |
| |
On Wed, Feb 23, 2022 at 05:00:12PM +0100, gregkh@linuxfoundation.org wrote: > On Wed, Feb 23, 2022 at 09:38:20AM -0500, stern@rowland.harvard.edu wrote: > > Which bus locks are you referring to? I'm not aware of any locks > > that synchronize dev_uevent() with anything (in particular, with > > driver unbinding). > > The locks in the driver core that handle the binding and unbinding of > drivers to devices. > > > And as far as I know, usb_gadget_remove_driver() doesn't play any > > odd tricks with pointers. > > Ah, I never noticed that this is doing a "fake" bus and does the > bind/unbind itself outside of the driver core. It should just be a > normal bus type and have the core do the work for it, but oh well. > > And there is a lock that should serialize all of this already, so it's > odd that this is able to be triggered at all.
>>I guess at a minimum the UDC core should hold the device lock when it registers, unregisters, binds, or unbinds UDC and gadget devices. >>Would that be enough to fix the problem? I really don't understand how sysfs file access gets synchronized with device removal.
>>> >>>Agree with you, in usb_gadget_remove_driver() function, the udc->dev.driver and udc->gadget->dev.driver be set to null without any protection, so when the udevd accessed the dev->driver, this address may be invalid at this time. >>>maybe the operation of dev->driver can be protected by device_lock(). >>>
Is it enough that we just need to protect "dev.driver" ?
diff --git a/drivers/base/core.c b/drivers/base/core.c index 3d6430eb0c6a..9bd2624973d7 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2316,8 +2316,10 @@ static int dev_uevent(struct kobject *kobj, struct kobj_uevent_env *env) if (dev->type && dev->type->name) add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
+ device_lock(dev); if (dev->driver) add_uevent_var(env, "DRIVER=%s", dev->driver->name); + device_unlock(dev);
/* Add common DT information about the device */ of_device_uevent(dev, env); diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 568534a0d17c..7877142397d3 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -1436,8 +1436,14 @@ static void usb_gadget_remove_driver(struct usb_udc *udc) usb_gadget_udc_stop(udc);
udc->driver = NULL; + + device_lock(&udc->dev); udc->dev.driver = NULL; + device_unlock(&udc->dev); + + device_lock(&udc->gadget->dev); udc->gadget->dev.driver = NULL; + device_unlock(&udc->gadget->dev); }
/** @@ -1498,8 +1504,14 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri driver->function);
udc->driver = driver; + + device_lock(&udc->dev); udc->dev.driver = &driver->driver; + device_unlock(&udc->dev); + + device_lock(&udc->gadget->dev); udc->gadget->dev.driver = &driver->driver; + device_unlock(&udc->gadget->dev);
usb_gadget_udc_set_speed(udc, driver->max_speed);
@@ -1521,8 +1533,14 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri dev_err(&udc->dev, "failed to start %s: %d\n", udc->driver->function, ret); udc->driver = NULL; + + device_lock(&udc->dev); udc->dev.driver = NULL; + device_unlock(&udc->dev); + + device_lock(&udc->gadget->dev); udc->gadget->dev.driver = NULL; + device_unlock(&udc->gadget->dev); return ret; }
Thanks, Zqiang
>>>Thanks, >>>Zqiang
> Unless the device is being removed at the same time it was manually > unbound from the driver? If so, then this really should be fixed up > to use the driver core logic instead... >> >>Device removal does of course trigger unbinding, but they always take place in the same thread so it isn't an issue. >> >>Probably part of the reason people don't want to use the driver core here is so that they can specify which UDC a gadget driver should bind to. The driver core would always bind each new gadget to the first registered gadget driver. >> >>When Dave Brownell originally wrote the gadget subsystem, I believe he didn't bother to integrate it with the driver core because it was a "bus" with only a single device and a single driver. The ability to have multiple UDCs in the system was added later. >> >>Alan Stern
| |