On 18/10/2024 09:15, Qiu-ji Chen wrote:
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@xxxxxxxxxxxxxxx
Signed-off-by: Qiu-ji Chen <chenqiuji666@xxxxxxxxx>
---
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;
}
Looks correct to me
Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
