linux-kernel - [PATCH] 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>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20241112163041.40083-1-chenqiuji666@gmail.com>
Date: Wed, 13 Nov 2024 00:30:41 +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] driver core: Fix concurrency issue in match driver interface.

This patch identifies a deeper issue. Taking the function
fsl_mc_bus_match() as an example, since the function is not protected by a
lock, there is a data race between fsl_mc_bus_match() and
driver_override_store(). After the check if (mc_dev->driver_override), the
driver_override field may be set to NULL, but the strcmp() function does
not perform a NULL pointer check. If a NULL driver_override field is passed
into strcmp(), it may lead to a null pointer dereference issue. We also
noticed that in the driver_override_store() function, the function
driver_set_override() is called, and this function frees the old value
after storing the new dev value. If a race occurs, a use-after-free (UAF)
issue could occur in fsl_mc_bus_match().

To fix this issue, we examined the interface that calls this function and
found that fsl_mc_bus_match() is called by the interface drv->bus->match,
which in turn is called by the function driver_match_device(). We found
three places where driver_match_device() is called, and the call in
__driver_attach() is locked. However, since the driver_override_store()
function is implemented in drivers such as amba, base, cdx, and others,
this data race issue is quite common. Adding a lock at the lower level
could conflict with the lock in __driver_attach(), leading to a potential
deadlock. Therefore, we decided to add locking to the other two calls to
driver_match_device() to ensure that when the function is called through
the match interface, dev is protected by a lock throughout the process, and
its value remains unchanged.

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