[PATCH openEuler-1.0-LTS 0/6] driver: base: fix UAF when driver_attach failed
The following series contains six patches that fix UAF when driver_attach failed: Alexander Duyck (2): driver core: Probe devices asynchronously instead of the driver driver core: Attach devices on CPU local to device node Jason Gunthorpe (1): driver core: Pull required checks into driver_probe_device() Mark-PK Tsai (1): driver core: Prevent overriding async driver of a device before it probe Schspa Shi (1): driver: base: fix UAF when driver_attach failed Zhang Wensheng (1): driver core: fix potential deadlock in __driver_attach drivers/base/base.h | 2 ++ drivers/base/bus.c | 27 +++++---------------- drivers/base/dd.c | 59 ++++++++++++++++++++++++++++++++++++--------- 3 files changed, 55 insertions(+), 33 deletions(-) -- 2.22.0
From: Alexander Duyck <alexander.h.duyck@linux.intel.com> mainline inclusion from mainline-v5.1-rc1 commit ef0ff68351be4fd83bec2d797f0efdc0174a55a4 category: bugfix bugzilla: https://gitee.com/src-openeuler/kernel/issues/IBP35K CVE: CVE-2022-49385 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... -------------------------------- Probe devices asynchronously instead of the driver. This results in us seeing the same behavior if the device is registered before the driver or after. This way we can avoid serializing the initialization should the driver not be loaded until after the devices have already been added. The motivation behind this is that if we have a set of devices that take a significant amount of time to load we can greatly reduce the time to load by processing them in parallel instead of one at a time. In addition, each device can exist on a different node so placing a single thread on one CPU to initialize all of the devices for a given driver can result in poor performance on a system with multiple nodes. This approach can reduce the time needed to scan SCSI LUNs significantly. The only way to realize that speedup is by enabling more concurrency which is what is achieved with this patch. To achieve this it was necessary to add a new member "async_driver" to the device_private structure to store the driver pointer while we wait on the deferred probe call. Reviewed-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Yin Tirui <yintirui@huawei.com> Reviewed-by: Weilong Chen <chenweilong@huawei.com> --- drivers/base/base.h | 2 ++ drivers/base/bus.c | 23 +++-------------------- drivers/base/dd.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/drivers/base/base.h b/drivers/base/base.h index 2d270b8c731a..92803ce31257 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -64,6 +64,7 @@ struct driver_private { * binding of drivers which were unable to get all the resources needed by * the device; typically because it depends on another driver getting * probed first. + * @async_driver - pointer to device driver awaiting probe via async_probe * @device - pointer back to the struct device that this structure is * associated with. * @dead - This device is currently either in the process of or has been @@ -78,6 +79,7 @@ struct device_private { struct klist_node knode_driver; struct klist_node knode_bus; struct list_head deferred_probe; + struct device_driver *async_driver; struct device *device; u8 dead:1; }; diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 5f1966081c42..07876b6aac57 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -611,17 +611,6 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf, } static DRIVER_ATTR_WO(uevent); -static void driver_attach_async(void *_drv, async_cookie_t cookie) -{ - struct device_driver *drv = _drv; - int ret; - - ret = driver_attach(drv); - - pr_debug("bus: '%s': driver %s async attach completed: %d\n", - drv->bus->name, drv->name, ret); -} - /** * bus_add_driver - Add a driver to the bus. * @drv: driver. @@ -654,15 +643,9 @@ int bus_add_driver(struct device_driver *drv) klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers); if (drv->bus->p->drivers_autoprobe) { - if (driver_allows_async_probing(drv)) { - pr_debug("bus: '%s': probing driver %s asynchronously\n", - drv->bus->name, drv->name); - async_schedule(driver_attach_async, drv); - } else { - error = driver_attach(drv); - if (error) - goto out_unregister; - } + error = driver_attach(drv); + if (error) + goto out_unregister; } module_add_driver(drv->owner, drv); diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 432a5645bac3..b69691f5d217 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -928,6 +928,30 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) return ret; } +static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) +{ + struct device *dev = _dev; + struct device_driver *drv; + int ret = 0; + + __device_driver_lock(dev, dev->parent); + + drv = dev->p->async_driver; + + /* + * If device has been removed or someone has already successfully + * bound a driver before us just skip the driver probe call. + */ + if (!dev->p->dead && !dev->driver) + ret = driver_probe_device(drv, dev); + + __device_driver_unlock(dev, dev->parent); + + dev_dbg(dev, "driver %s async attach completed: %d\n", drv->name, ret); + + put_device(dev); +} + static int __driver_attach(struct device *dev, void *data) { struct device_driver *drv = data; @@ -964,6 +988,25 @@ static int __driver_attach(struct device *dev, void *data) return 0; } /* ret > 0 means positive match */ + if (driver_allows_async_probing(drv)) { + /* + * Instead of probing the device synchronously we will + * probe it asynchronously to allow for more parallelism. + * + * We only take the device lock here in order to guarantee + * that the dev->driver and async_driver fields are protected + */ + dev_dbg(dev, "probing driver %s asynchronously\n", drv->name); + device_lock(dev); + if (!dev->driver) { + get_device(dev); + dev->p->async_driver = drv; + async_schedule(__driver_attach_async_helper, dev); + } + device_unlock(dev); + return 0; + } + device_driver_attach(drv, dev); return 0; -- 2.22.0
From: Schspa Shi <schspa@gmail.com> mainline inclusion from mainline-v5.19-rc1 commit 310862e574001a97ad02272bac0fd13f75f42a27 category: bugfix bugzilla: https://gitee.com/src-openeuler/kernel/issues/IBP35K CVE: CVE-2022-49385 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... -------------------------------- When driver_attach(drv); failed, the driver_private will be freed. But it has been added to the bus, which caused a UAF. To fix it, we need to delete it from the bus when failed. Fixes: 190888ac01d0 ("driver core: fix possible missing of device probe") Signed-off-by: Schspa Shi <schspa@gmail.com> Link: https://lore.kernel.org/r/20220513112444.45112-1-schspa@gmail.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Yin Tirui <yintirui@huawei.com> Reviewed-by: Weilong Chen <chenweilong@huawei.com> --- drivers/base/bus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 07876b6aac57..35b7fe25e0a4 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -645,7 +645,7 @@ int bus_add_driver(struct device_driver *drv) if (drv->bus->p->drivers_autoprobe) { error = driver_attach(drv); if (error) - goto out_unregister; + goto out_del_list; } module_add_driver(drv->owner, drv); @@ -672,6 +672,8 @@ int bus_add_driver(struct device_driver *drv) return 0; +out_del_list: + klist_del(&priv->knode_bus); out_unregister: kobject_put(&priv->kobj); /* drv->p is freed in driver_release() */ -- 2.22.0
From: Alexander Duyck <alexander.h.duyck@linux.intel.com> mainline inclusion from mainline-v5.1-rc1 commit c37e20eaf4b21125898fd454f3ea6b212865d0a6 category: bugfix bugzilla: https://gitee.com/src-openeuler/kernel/issues/IBP35K CVE: CVE-2022-49385 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... -------------------------------- Call the asynchronous probe routines on a CPU local to the device node. By doing this we should be able to improve our initialization time significantly as we can avoid having to access the device from a remote node which may introduce higher latency. For example, in the case of initializing memory for NVDIMM this can have a significant impact as initialing 3TB on remote node can take up to 39 seconds while initialing it on a local node only takes 23 seconds. It is situations like this where we will see the biggest improvement. Reviewed-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Yin Tirui <yintirui@huawei.com> Reviewed-by: Weilong Chen <chenweilong@huawei.com> --- drivers/base/dd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index b69691f5d217..76a40017bb7f 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -832,7 +832,7 @@ static int __device_attach(struct device *dev, bool allow_async) */ dev_dbg(dev, "scheduling asynchronous probe\n"); get_device(dev); - async_schedule(__device_attach_async_helper, dev); + async_schedule_dev(__device_attach_async_helper, dev); } else { pm_request_idle(dev); } @@ -1001,7 +1001,7 @@ static int __driver_attach(struct device *dev, void *data) if (!dev->driver) { get_device(dev); dev->p->async_driver = drv; - async_schedule(__driver_attach_async_helper, dev); + async_schedule_dev(__driver_attach_async_helper, dev); } device_unlock(dev); return 0; -- 2.22.0
From: Jason Gunthorpe <jgg@nvidia.com> mainline inclusion from mainline-v5.14-rc1 commit 204db60c83574559a8e08ce4bbd0029d56b8ab2e category: bugfix bugzilla: https://gitee.com/src-openeuler/kernel/issues/IBP35K CVE: CVE-2022-49385 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... -------------------------------- Checking if the dev is dead or if the dev is already bound is a required precondition to invoking driver_probe_device(). All the call chains leading here duplicate these checks. Add it directly to driver_probe_device() so the precondition is clear and remove the checks from device_driver_attach() and __driver_attach_async_helper(). The other call chain going through __device_attach_driver() does have these same checks but they are inlined into logic higher up the call stack and can't be removed. The sysfs uAPI call chain starting at bind_store() is a bit confused because it reads dev->driver unlocked and returns -ENODEV if it is !NULL, otherwise it reads it again under lock and returns 0 if it is !NULL. Fix this to always return -EBUSY and always read dev->driver under its lock. Done in preparation for the next patches which will add additional callers to driver_probe_device() and will need these checks as well. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> [hch: drop the extra checks in device_driver_attach and bind_store] Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Reviewed-by: Cornelia Huck <cohuck@redhat.com> Link: https://lore.kernel.org/r/20210617142218.1877096-2-hch@lst.de Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Yin Tirui <yintirui@huawei.com> Reviewed-by: Weilong Chen <chenweilong@huawei.com> --- drivers/base/bus.c | 2 +- drivers/base/dd.c | 32 ++++++++++---------------------- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 35b7fe25e0a4..882a703b5c21 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -209,7 +209,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf, int err = -ENODEV; dev = bus_find_device_by_name(bus, NULL, buf); - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) { + if (dev && driver_match_device(drv, dev)) { err = device_driver_attach(drv, dev); if (err > 0) { diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 76a40017bb7f..17bcc0e7f710 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -634,8 +634,9 @@ EXPORT_SYMBOL_GPL(wait_for_device_probe); * @drv: driver to bind a device to * @dev: device to try to bind to the driver * - * This function returns -ENODEV if the device is not registered, - * 1 if the device is bound successfully and 0 otherwise. + * This function returns -ENODEV if the device is not registered, -EBUSY if it + * already has a driver, and 1 if the device is bound successfully and 0 + * otherwise. * * This function must be called with @dev lock held. When called for a * USB interface, @dev->parent lock must be held as well. @@ -646,8 +647,10 @@ int driver_probe_device(struct device_driver *drv, struct device *dev) { int ret = 0; - if (!device_is_registered(dev)) + if (dev->p->dead || !device_is_registered(dev)) return -ENODEV; + if (dev->driver) + return -EBUSY; pr_debug("bus: '%s': %s: matched device %s with driver %s\n", drv->bus->name, __func__, dev_name(dev), drv->name); @@ -912,17 +915,10 @@ static void __device_driver_unlock(struct device *dev, struct device *parent) */ int device_driver_attach(struct device_driver *drv, struct device *dev) { - int ret = 0; + int ret; __device_driver_lock(dev, dev->parent); - - /* - * If device has been removed or someone has already successfully - * bound a driver before us just skip the driver probe call. - */ - if (!dev->p->dead && !dev->driver) - ret = driver_probe_device(drv, dev); - + ret = driver_probe_device(drv, dev); __device_driver_unlock(dev, dev->parent); return ret; @@ -932,19 +928,11 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) { struct device *dev = _dev; struct device_driver *drv; - int ret = 0; + int ret; __device_driver_lock(dev, dev->parent); - drv = dev->p->async_driver; - - /* - * If device has been removed or someone has already successfully - * bound a driver before us just skip the driver probe call. - */ - if (!dev->p->dead && !dev->driver) - ret = driver_probe_device(drv, dev); - + ret = driver_probe_device(drv, dev); __device_driver_unlock(dev, dev->parent); dev_dbg(dev, "driver %s async attach completed: %d\n", drv->name, ret); -- 2.22.0
From: Mark-PK Tsai <mark-pk.tsai@mediatek.com> mainline inclusion from mainline-v5.19-rc1 commit 84e7c6786aad1dffa04f5729270f8fcd7281fe4b category: bugfix bugzilla: https://gitee.com/src-openeuler/kernel/issues/IBP35K CVE: CVE-2022-49385 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... -------------------------------- When there are 2 matched drivers for a device using async probe mechanism, the dev->p->async_driver might be overridden by the last attached driver. So just skip the later one if the previous matched driver was not handled by async thread yet. Below is my use case which having this problem. Make both driver mmcblk and mmc_test allow async probe, the dev->p->async_driver will be overridden by the later driver mmc_test and bind to the device then claim it for testing. When it happen, mmcblk will never do probe again. Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com> Link: https://lore.kernel.org/r/20220316074328.1801-1-mark-pk.tsai@mediatek.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Yin Tirui <yintirui@huawei.com> Reviewed-by: Weilong Chen <chenweilong@huawei.com> --- drivers/base/dd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 17bcc0e7f710..24b7d8763131 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -932,6 +932,7 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) __device_driver_lock(dev, dev->parent); drv = dev->p->async_driver; + dev->p->async_driver = NULL; ret = driver_probe_device(drv, dev); __device_driver_unlock(dev, dev->parent); @@ -986,7 +987,7 @@ static int __driver_attach(struct device *dev, void *data) */ dev_dbg(dev, "probing driver %s asynchronously\n", drv->name); device_lock(dev); - if (!dev->driver) { + if (!dev->driver && !dev->p->async_driver) { get_device(dev); dev->p->async_driver = drv; async_schedule_dev(__driver_attach_async_helper, dev); -- 2.22.0
From: Zhang Wensheng <zhangwensheng5@huawei.com> mainline inclusion from mainline-v6.0-rc1 commit 70fe758352cafdee72a7b13bf9db065f9613ced8 category: bugfix bugzilla: https://gitee.com/src-openeuler/kernel/issues/IBP35K CVE: CVE-2022-49385 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=... -------------------------------- In __driver_attach function, There are also AA deadlock problem, like the commit b232b02bf3c2 ("driver core: fix deadlock in __device_attach"). stack like commit b232b02bf3c2 ("driver core: fix deadlock in __device_attach"). list below: In __driver_attach function, The lock holding logic is as follows: ... __driver_attach if (driver_allows_async_probing(drv)) device_lock(dev) // get lock dev async_schedule_dev(__driver_attach_async_helper, dev); // func async_schedule_node async_schedule_node_domain(func) entry = kzalloc(sizeof(struct async_entry), GFP_ATOMIC); /* when fail or work limit, sync to execute func, but __driver_attach_async_helper will get lock dev as will, which will lead to A-A deadlock. */ if (!entry || atomic_read(&entry_count) > MAX_WORK) { func; else queue_work_node(node, system_unbound_wq, &entry->work) device_unlock(dev) As above show, when it is allowed to do async probes, because of out of memory or work limit, async work is not be allowed, to do sync execute instead. it will lead to A-A deadlock because of __driver_attach_async_helper getting lock dev. Reproduce: and it can be reproduce by make the condition (if (!entry || atomic_read(&entry_count) > MAX_WORK)) untenable, like below: [ 370.785650] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 370.787154] task:swapper/0 state:D stack: 0 pid: 1 ppid: 0 flags:0x00004000 [ 370.788865] Call Trace: [ 370.789374] <TASK> [ 370.789841] __schedule+0x482/0x1050 [ 370.790613] schedule+0x92/0x1a0 [ 370.791290] schedule_preempt_disabled+0x2c/0x50 [ 370.792256] __mutex_lock.isra.0+0x757/0xec0 [ 370.793158] __mutex_lock_slowpath+0x1f/0x30 [ 370.794079] mutex_lock+0x50/0x60 [ 370.794795] __device_driver_lock+0x2f/0x70 [ 370.795677] ? driver_probe_device+0xd0/0xd0 [ 370.796576] __driver_attach_async_helper+0x1d/0xd0 [ 370.797318] ? driver_probe_device+0xd0/0xd0 [ 370.797957] async_schedule_node_domain+0xa5/0xc0 [ 370.798652] async_schedule_node+0x19/0x30 [ 370.799243] __driver_attach+0x246/0x290 [ 370.799828] ? driver_allows_async_probing+0xa0/0xa0 [ 370.800548] bus_for_each_dev+0x9d/0x130 [ 370.801132] driver_attach+0x22/0x30 [ 370.801666] bus_add_driver+0x290/0x340 [ 370.802246] driver_register+0x88/0x140 [ 370.802817] ? virtio_scsi_init+0x116/0x116 [ 370.803425] scsi_register_driver+0x1a/0x30 [ 370.804057] init_sd+0x184/0x226 [ 370.804533] do_one_initcall+0x71/0x3a0 [ 370.805107] kernel_init_freeable+0x39a/0x43a [ 370.805759] ? rest_init+0x150/0x150 [ 370.806283] kernel_init+0x26/0x230 [ 370.806799] ret_from_fork+0x1f/0x30 To fix the deadlock, move the async_schedule_dev outside device_lock, as we can see, in async_schedule_node_domain, the parameter of queue_work_node is system_unbound_wq, so it can accept concurrent operations. which will also not change the code logic, and will not lead to deadlock. Fixes: ef0ff68351be ("driver core: Probe devices asynchronously instead of the driver") Signed-off-by: Zhang Wensheng <zhangwensheng5@huawei.com> Link: https://lore.kernel.org/r/20220622074327.497102-1-zhangwensheng5@huawei.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Yin Tirui <yintirui@huawei.com> Reviewed-by: Weilong Chen <chenweilong@huawei.com> --- drivers/base/dd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 24b7d8763131..38fed70f500f 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -944,6 +944,7 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) static int __driver_attach(struct device *dev, void *data) { struct device_driver *drv = data; + bool async = false; int ret; /* @@ -990,9 +991,11 @@ static int __driver_attach(struct device *dev, void *data) if (!dev->driver && !dev->p->async_driver) { get_device(dev); dev->p->async_driver = drv; - async_schedule_dev(__driver_attach_async_helper, dev); + async = true; } device_unlock(dev); + if (async) + async_schedule_dev(__driver_attach_async_helper, dev); return 0; } -- 2.22.0
反馈： 您发送到kernel@openeuler.org的补丁/补丁集，已成功转换为PR！ PR链接地址： https://gitee.com/openeuler/kernel/pulls/16118 邮件列表地址：https://mailweb.openeuler.org/archives/list/kernel@openeuler.org/message/KX7... FeedBack: The patch(es) which you have sent to kernel@openeuler.org mailing list has been converted to a pull request successfully! Pull request link: https://gitee.com/openeuler/kernel/pulls/16118 Mailing list address: https://mailweb.openeuler.org/archives/list/kernel@openeuler.org/message/KX7...
participants (2)
-
patchwork bot -
Yin Tirui