linux-kernel - [PATCH] cdx: Fix atomicity violation in cdx_bus_match() and cdx

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: <20241018081636.1379390-1-chenqiuji666@gmail.com>
Date: Fri, 18 Oct 2024 16:16:36 +0800
From: Qiu-ji Chen <chenqiuji666@...il.com>
To: nipun.gupta@....com,
	nikhil.agarwal@....com
Cc: linux-kernel@...r.kernel.org,
	baijiaju1990@...il.com,
	Qiu-ji Chen <chenqiuji666@...il.com>,
	stable@...r.kernel.org
Subject: [PATCH] cdx: Fix atomicity violation in cdx_bus_match() and cdx_probe()

An atomicity violation occurs during consecutive reads of the variable 
cdx_dev->driver_override. Imagine a scenario: while evaluating the 
statement if (cdx_dev->driver_override && strcmp(cdx_dev->driver_override, 
drv->name)), the value of cdx_dev->driver_override changes, leading to an 
inconsistency where the value of cdx_dev->driver_override is the old value 
when passing the non-null check, but the new value when evaluated by 
strcmp(). This causes an inconsistency.

The second error occurs during the validation of cdx_dev->driver_override. 
The logic of this error is similar to the first one, as the entire process 
is not protected by a lock, leading to an inconsistency in the values of 
cdx_dev->driver_override before and after the reads.

The third error occurs in driver_override_show() when executing the 
statement return sysfs_emit(buf, "%s\n", cdx_dev->driver_override);. 
Since the string changes byte by byte, it is possible for a partially 
modified cdx_dev->driver_override value to be used in this statement, 
leading to an incorrect return value from the program.

To fix these issues, for the first and second problems, since we need to 
protect the entire process of reading the variable cdx_dev->driver_override
with a lock, we introduced a variable ret and an out block. For each branch
in this section, we replaced the return statements with assignments to the
variable ret, and then used a goto statement to directly execute the out 
block, making the code overall more concise.

For the third problem, we adopted a similar approach to the one used in the
modalias_show() function, protecting the process of reading 
cdx_dev->driver_override with a lock, ensuring that the program runs 
correctly.

This possible bug is found by an experimental static analysis tool
developed by our team. This tool analyzes the locking APIs to extract
function pairs that can be concurrently executed, and then analyzes the
instructions in the paired functions to identify possible concurrency bugs
including data races and atomicity violations.

Fixes: 2959ab247061 ("cdx: add the cdx bus driver")
Fixes: 48a6c7bced2a ("cdx: add device attributes")
Cc: stable@...r.kernel.org
Signed-off-by: Qiu-ji Chen <chenqiuji666@...il.com>
---
 drivers/cdx/cdx.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index 07371cb653d3..fae03c89f818 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -268,6 +268,7 @@ static int cdx_bus_match(struct device *dev, const struct device_driver *drv)
 	const struct cdx_driver *cdx_drv = to_cdx_driver(drv);
 	const struct cdx_device_id *found_id = NULL;
 	const struct cdx_device_id *ids;
+	int ret = false;

 	if (cdx_dev->is_bus)
 		return false;
@@ -275,28 +276,40 @@ static int cdx_bus_match(struct device *dev, const struct device_driver *drv)
 	ids = cdx_drv->match_id_table;

 	/* When driver_override is set, only bind to the matching driver */
-	if (cdx_dev->driver_override && strcmp(cdx_dev->driver_override, drv->name))
-		return false;
+	device_lock(dev);
+	if (cdx_dev->driver_override && strcmp(cdx_dev->driver_override, drv->name)) {
+		ret = false;
+		goto out;
+	}

 	found_id = cdx_match_id(ids, cdx_dev);
-	if (!found_id)
-		return false;
+	if (!found_id) {
+		ret = false;
+		goto out;
+	}

 	do {
 		/*
 		 * In case override_only was set, enforce driver_override
 		 * matching.
 		 */
-		if (!found_id->override_only)
-			return true;
-		if (cdx_dev->driver_override)
-			return true;
+		if (!found_id->override_only) {
+			ret = true;
+			goto out;
+		}
+		if (cdx_dev->driver_override) {
+			ret = true;
+			goto out;
+		}

 		ids = found_id + 1;
 		found_id = cdx_match_id(ids, cdx_dev);
 	} while (found_id);

-	return false;
+	ret = false;
+out:
+	device_unlock(dev);
+	return ret;
 }

 static int cdx_probe(struct device *dev)
@@ -470,8 +483,12 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct cdx_device *cdx_dev = to_cdx_device(dev);
+	ssize_t len;

-	return sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
+	device_lock(dev);
+	len = sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
+	device_unlock(dev);
+	return len;
 }
 static DEVICE_ATTR_RW(driver_override);

-- 
2.34.1