linux-kernel - RE: [syzbot] KASAN: use-after-free Read in dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PH0PR11MB588091026B817203C772B264DA3D9@PH0PR11MB5880.namprd11.prod.outlook.com>
Date:   Thu, 24 Feb 2022 03:14:54 +0000
From:   "Zhang, Qiang1" <qiang1.zhang@...el.com>
To:     "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     syzbot <syzbot+348b571beb5eeb70a582@...kaller.appspotmail.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "syzkaller-bugs@...glegroups.com" <syzkaller-bugs@...glegroups.com>,
        "balbi@...nel.org" <balbi@...nel.org>
Subject: RE: [syzbot] KASAN: use-after-free Read in dev_uevent


On Wed, Feb 23, 2022 at 05:00:12PM +0100, gregkh@...uxfoundation.org wrote:
> On Wed, Feb 23, 2022 at 09:38:20AM -0500, stern@...land.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