RE: [syzbot] KASAN: use-after-free Read in dev_uevent

From: "Zhang, Qiang1" <qiang1.zhang@intel.com>
To: "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: syzbot <syzbot+348b571beb5eeb70a582@syzkaller.appspotmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"rafael@kernel.org" <rafael@kernel.org>,
	"syzkaller-bugs@googlegroups.com"
	<syzkaller-bugs@googlegroups.com>,
	"balbi@kernel.org" <balbi@kernel.org>
Subject: RE: [syzbot] KASAN: use-after-free Read in dev_uevent
Date: Thu, 24 Feb 2022 01:44:11 +0000	[thread overview]
Message-ID: <PH0PR11MB5880D7544442B4D60810F0D2DA3D9@PH0PR11MB5880.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YhZiMHHjrBw8am5g@rowland.harvard.edu>

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(). 

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