Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
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
Thread-Topic: [syzbot] KASAN: use-after-free Read in dev_uevent
Thread-Index: AQHYJn4xBH205WNMF0CseYQWl0QGTKyg/zWggAAE7YCAADTiAIAAFuAAgAAJtACAAJZv8IAAG7Gw
Date:   Thu, 24 Feb 2022 03:14:54 +0000
Message-ID: <PH0PR11MB588091026B817203C772B264DA3D9@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <0000000000005a991a05a86970bb@google.com>
 <00000000000033314805d8765175@google.com>
 <PH0PR11MB58805E3C4CF7D4C41D49BFCFDA3C9@PH0PR11MB5880.namprd11.prod.outlook.com>
 <YhYafwiwUV2Sbn5t@kroah.com> <YhZG3GJb8G7oL7l7@rowland.harvard.edu>
 <YhZaDGeIIvpILdCk@kroah.com> <YhZiMHHjrBw8am5g@rowland.harvard.edu>
 <PH0PR11MB5880D7544442B4D60810F0D2DA3D9@PH0PR11MB5880.namprd11.prod.outlook.com>
In-Reply-To: <PH0PR11MB5880D7544442B4D60810F0D2DA3D9@PH0PR11MB5880.namprd11.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.6.200.16
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?eMarFR8xxI8kEMlr6peI4TNfK0uE1lh4uMnPNorz9tsntq1gfTlv15UQKMXN?=
 =?us-ascii?Q?nbtDGA9A4hS4nly02I2PN6MEdxABpBAarDJ7Q7bcwzNgW6CIWX1AU6qm0WXK?=
 =?us-ascii?Q?iX658N/8O67K5tSXhqv7to92JZGqJiIHyltl6ZJTrx+SDZ0FqtKUzGPJQiIF?=
 =?us-ascii?Q?3/BDs37S+EXCcRKiDJB4UnGaDytxJmfTVMUHAEACPSI/wuCuMFomH6Ud1W27?=
 =?us-ascii?Q?gMua6xSOCf75S3pwzMt4n+CN7mttkuxugOJyBIKFqqjyFLgZnOM9zSvl/Qvd?=
 =?us-ascii?Q?/CLtemvtBjrPNLLADBwrTsyn+JlxbsUjSIY2JDs6gjVZm+EaMn8pvjWfmHS4?=
 =?us-ascii?Q?ZdzJRt3ysi1a8DNj6vhmY7jsH/Y7RZ3gF39f9noF9O+l0DkBfSxERxww1Agx?=
 =?us-ascii?Q?2LKSRA1cn43M+SCFoJUxhRF1BSw9K/bkbjfQSnYe97c5jMHmUTVKfzzctQsv?=
 =?us-ascii?Q?1y7AZrP4gxo/UjXKBuhZlEqzM5kNjZAr89i1nbC9Uj0WaIwUaOribq+gGaAQ?=
 =?us-ascii?Q?xRh8PWRYqTH3NTzJXU/gJ0RcqDsOHkS83g/k3wltTxsdEU4WWYEc0N2nLOEf?=
 =?us-ascii?Q?/Oh1eKPq+bAqxAWQy7auCSYlIleSIFtFYhVZvYdROO6BQnfnd4iw+b2LQlYr?=
 =?us-ascii?Q?tn0acpSxtHsw+AvFiypGtEj8fuLjHHLc+gZPT0I+iKulWgASCNW3vz2l4fYX?=
 =?us-ascii?Q?qbSFriP+6FYRm+6QgJv87323fwGf2d4KvTwI+yeMCxWPvzNoOH5+8tTFHd68?=
 =?us-ascii?Q?dlkEYLTDMpW1jKQ9i9VPyC/6KQA/SwQMHWejj45VWE28V4BDiBD0he/v8ncD?=
 =?us-ascii?Q?nx01uV1ri5v9nLFWv8jS8zBdpDqq5JqFaZKjOQUbUmhyBgSA3g6lxVpwqcdK?=
 =?us-ascii?Q?OQvTYvzsB79AVrUareEOdcOXt6dLbv9ZS4p9RqA8h0yuYLfZdGPbwW3Zxw5I?=
 =?us-ascii?Q?uqBtQGzpF0rPGUltN7RcoRfAgVG5I5vUVNL6mOkas3kPHIEF2JXy6e6SoOdH?=
 =?us-ascii?Q?FM9oSXOc6culg7YWU2HqUiNnKmvJFf/36BTLy1G0svytpb9vFz/0dU6UAjfV?=
 =?us-ascii?Q?2G+E8AJwrjB5SIPTNvs33Jl4zi2Kv0nZP0WEfkahCljjcuw2efWGQdUsSMa2?=
 =?us-ascii?Q?88hRm1aYiGh0okNP4PAwf5flNXu5oF+/b+XDGEl2/gcGt07zdIDE1Jq1B+uO?=
 =?us-ascii?Q?q3P1A/IVW7fZujbqi1IQaPYDlgwGG7EflVOCDALdHXL+PmD1/1dnbsNi0EIW?=
 =?us-ascii?Q?tD3bL6wmetnyLm89a7zGzrHagEEHPjdPUcLq0r6Kg2cSs56h4P3H93Ihe6Q4?=
 =?us-ascii?Q?cSnz+J2z+FMQCk/+jZl6t8n4dMmieY3Z8d261Vvbg4ftEQJACsb+kuD3u1TK?=
 =?us-ascii?Q?RMgyxphuJxMg1g/FtaBJ7Ab0a2oPALeRSAJky+85en75ruebulkFEBLq2ULV?=
 =?us-ascii?Q?SmMMIuhFb1WhZSt4tPGookzXMF0XFVdMUgw+uPpZltwj6aRgFtUx5oF8R1GI?=
 =?us-ascii?Q?wuUJuO+7zzjYErE/1BoqoruzV32WhVZrRryn9gIo26pWfq8vrVGQMl3tcgL7?=
 =?us-ascii?Q?SLAbcxCSS3eCoskF8L61cHX6Bv5BLHJQWgpBFSWTgZLePxGxdlDMtbEU1HOK?=
 =?us-ascii?Q?j9DcKNRSwyuHzlNbxh2icCI=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5880.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b0bf1500-12f0-4cfa-0ca2-08d9f743d484
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Feb 2022 03:14:54.8666
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 2s5X/4k/9RzCLjjHOfMkiC6YMJyzISneOm1nBo3/dO8EEGzlzliKJZ6na9QJHhSTPSsO3vEZllk5Gkv8l7Q6zA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB0060
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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=20
> > that synchronize dev_uevent() with anything (in particular, with=20
> > driver unbinding).
>=20
> The locks in the driver core that handle the binding and unbinding of=20
> drivers to devices.
>=20
> > And as far as I know, usb_gadget_remove_driver() doesn't play any=20
> > odd tricks with pointers.
>=20
> Ah, I never noticed that this is doing a "fake" bus and does the=20
> bind/unbind itself outside of the driver core.  It should just be a=20
> normal bus type and have the core do the work for it, but oh well.
>=20
> And there is a lock that should serialize all of this already, so it's=20
> 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 reg=
isters, unregisters, binds, or unbinds UDC and gadget devices. =20
>>Would that be enough to fix the problem?  I really don't understand how s=
ysfs file access gets synchronized with device removal.

>>>
>>>Agree with you, in usb_gadget_remove_driver() function, the udc->dev.dri=
ver and udc->gadget->dev.driver be set to null without any protection, so w=
hen 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().=20
>>>

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 k=
obj_uevent_env *env)
        if (dev->type && dev->type->name)
                add_uevent_var(env, "DEVTYPE=3D%s", dev->type->name);

+       device_lock(dev);
        if (dev->driver)
                add_uevent_var(env, "DRIVER=3D%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 =3D NULL;
+
+       device_lock(&udc->dev);
        udc->dev.driver =3D NULL;
+       device_unlock(&udc->dev);
+
+       device_lock(&udc->gadget->dev);
        udc->gadget->dev.driver =3D NULL;
+       device_unlock(&udc->gadget->dev);
 }

 /**
@@ -1498,8 +1504,14 @@ static int udc_bind_to_driver(struct usb_udc *udc, s=
truct usb_gadget_driver *dri
                        driver->function);

        udc->driver =3D driver;
+
+       device_lock(&udc->dev);
        udc->dev.driver =3D &driver->driver;
+       device_unlock(&udc->dev);
+
+       device_lock(&udc->gadget->dev);
        udc->gadget->dev.driver =3D &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, s=
truct usb_gadget_driver *dri
                dev_err(&udc->dev, "failed to start %s: %d\n",
                        udc->driver->function, ret);
        udc->driver =3D NULL;
+
+       device_lock(&udc->dev);
        udc->dev.driver =3D NULL;
+       device_unlock(&udc->dev);
+
+       device_lock(&udc->gadget->dev);
        udc->gadget->dev.driver =3D 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=20
> unbound from the driver?  If so, then this really should be fixed up=20
> to use the driver core logic instead...
>>
>>Device removal does of course trigger unbinding, but they always take pla=
ce 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 gadg=
et driver.
>>
>>When Dave Brownell originally wrote the gadget subsystem, I believe he di=
dn't bother to integrate it with the driver core because it was a "bus" wit=
h only a single device and a single driver.  The ability to have multiple U=
DCs in the system was added later.
>>
>>Alan Stern