linux-kernel - [PATCH] amba: Fix atomicity violation in amba

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: <20241018081539.1358921-1-chenqiuji666@gmail.com>
Date: Fri, 18 Oct 2024 16:15:39 +0800
From: Qiu-ji Chen <chenqiuji666@...il.com>
To: linux@...linux.org.uk,
	suzuki.poulose@....com,
	rmk+kernel@...linux.org.uk,
	sumit.garg@...aro.org,
	gregkh@...uxfoundation.org,
	andi.shyti@...nel.org,
	krzk@...nel.org
Cc: linux-kernel@...r.kernel.org,
	baijiaju1990@...il.com,
	Qiu-ji Chen <chenqiuji666@...il.com>,
	stable@...r.kernel.org
Subject: [PATCH] amba: Fix atomicity violation in amba_match()

Atomicity violation occurs during consecutive reads of 
pcdev->driver_override. Consider a scenario: after pvdev->driver_override 
passes the if statement, due to possible concurrency, 
pvdev->driver_override may change. This leads to pvdev->driver_override 
passing the condition with an old value, but entering the 
return !strcmp(pcdev->driver_override, drv->name); statement with a new 
value. This causes the function to return an unexpected result. 
Since pvdev->driver_override is a string that is modified byte by byte, 
without considering atomicity, data races may cause a partially modified 
pvdev->driver_override to enter both the condition and return statements, 
resulting in an error.

To fix this, we suggest protecting all reads of pvdev->driver_override 
with a lock, and storing the result of the strcmp() function in a new 
variable retval. This ensures that pvdev->driver_override does not change 
during the entire operation, allowing the function to return the expected 
result.

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: 5150a8f07f6c ("amba: reorder functions")
Cc: stable@...r.kernel.org
Signed-off-by: Qiu-ji Chen <chenqiuji666@...il.com>
---
 drivers/amba/bus.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 34bc880ca20b..e310f4f83b27 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -209,6 +209,7 @@ static int amba_match(struct device *dev, const struct device_driver *drv)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	const struct amba_driver *pcdrv = to_amba_driver(drv);
+	int retval;

 	mutex_lock(&pcdev->periphid_lock);
 	if (!pcdev->periphid) {
@@ -230,8 +231,14 @@ static int amba_match(struct device *dev, const struct device_driver *drv)
 	mutex_unlock(&pcdev->periphid_lock);

 	/* When driver_override is set, only bind to the matching driver */
-	if (pcdev->driver_override)
-		return !strcmp(pcdev->driver_override, drv->name);
+
+	device_lock(dev);
+	if (pcdev->driver_override) {
+		retval = !strcmp(pcdev->driver_override, drv->name);
+		device_unlock(dev);
+		return retval;
+	}
+	device_unlock(dev);

 	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
 }
-- 
2.34.1