linux-kernel - [PATCH v2] driver core: Fix concurrency issue in match driver interface

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>] [day] [month] [year] [list]

Message-Id: <20241112181733.148068-1-chenqiuji666@gmail.com>
Date: Wed, 13 Nov 2024 02:17:33 +0800
From: Qiu-ji Chen <chenqiuji666@...il.com>
To: gregkh@...uxfoundation.org,
	rafael@...nel.org
Cc: linux-kernel@...r.kernel.org,
	baijiaju1990@...il.com,
	Qiu-ji Chen <chenqiuji666@...il.com>
Subject: [PATCH v2] driver core: Fix concurrency issue in match driver interface

This patch identifies a concurrency issue. Taking the fsl_mc_bus_match()
function as an example, fsl_mc_bus_match() is an implementation of the
drv->bus->match interface. Since the entire function is not protected by a
lock, it can experience a data race with the driver_set_override()
function. The driver_set_override() function is provided by the driver
core, and in this function, the driver_override field is updated while
holding the device_lock.

After the check if (mc_dev->driver_override), the driver_override field may
be set to NULL, but the strcmp() function does not handle the case of a
NULL pointer. If the empty driver_override field is passed to strcmp(), it
could lead to a null pointer dereference issue. Additionally, we noticed
that in the driver_set_override() function, after storing the new dev
value, the old value is released. If a race condition occurs, this could
lead to a use-after-free (UAF) issue in fsl_mc_bus_match().

The only reference to fsl_mc_bus_match() is through the drv->bus->match
interface, which is a typical example. Many drivers, such as amba, base,
cdx, and others, have similar implementations.

To fix this issue, we examined the interface that calls this function and
found that the fsl_mc_bus_match() function is called by the drv->bus->match
interface, which in turn is called by the driver_match_device() function.
We discovered that there are three places in the code where
driver_match_device() is called. In the call chain from __device_attach to
__device_attach_driver to driver_match_device, the call to
__device_attach_driver within __device_attach is already protected by a
lock, so the call to driver_match_device in this chain is locked.

Since the driver_set_override() function, as mentioned earlier, is called
in several drivers, including amba, base, cdx, and others, this data race
issue is relatively common. Adding a lock at the lower level would conflict
with the lock in the upper-level __device_attach(), leading to a potential
deadlock. Therefore, we decided to add locks at the other two places where
driver_match_device() is called to ensure that when the match interface is
invoked, it prevents changes to the driver_override field during the call
to driver_set_override(), thus resolving the data race issue.

Fixes: 49b420a13ff9 ("driver core: check bus->match without holding device lock")
Signed-off-by: Qiu-ji Chen <chenqiuji666@...il.com>
---
In 2008, the kernel moved all match checks outside of the lock, and at that
time, there was no override, so this approach worked. In 2015, one function
moved the match check into the lock on one of its paths in order to support
asynchronous device binding, which led to inconsistent behavior. Later,
some kernel drivers added the driver_override feature to support binding
specific drivers, and the driver_set_override function was widely used to
control it. This function uses device_lock to control concurrency. This
issue is caused by both lower-level drivers and upper-level drivers, so
I'm not sure if the fixes tag is correct, and I haven't added a cc. I hope
to discuss this issue with the developers.
---
V2:
The incorrect description have been fixed.
Thanks Greg KH for helpful suggestion.
---
 drivers/base/bus.c | 16 +++++++++++-----
 drivers/base/dd.c  |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 657c93c38b0d..78d8c58fc50c 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -261,13 +261,19 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 	const struct bus_type *bus = bus_get(drv->bus);
 	struct device *dev;
 	int err = -ENODEV;
+	int ret;

 	dev = bus_find_device_by_name(bus, NULL, buf);
-	if (dev && driver_match_device(drv, dev)) {
-		err = device_driver_attach(drv, dev);
-		if (!err) {
-			/* success */
-			err = count;
+	if (dev) {
+		device_lock(dev);
+		ret = driver_match_device(drv, dev);
+		device_unlock(dev);
+		if (ret) {
+			err = device_driver_attach(drv, dev);
+			if (!err) {
+				/* success */
+				err = count;
+			}
 		}
 	}
 	put_device(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f0e4b4aba885..0b894719eb28 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1169,7 +1169,9 @@ static int __driver_attach(struct device *dev, void *data)
 	 * is an error.
 	 */

+	device_lock(dev);
 	ret = driver_match_device(drv, dev);
+	device_unlock(dev);
 	if (ret == 0) {
 		/* no match */
 		return 0;
-- 
2.34.1