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@vger.kernel.org Signed-off-by: Qiu-ji Chen chenqiuji666@gmail.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; }
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@vger.kernel.org Signed-off-by: Qiu-ji Chen chenqiuji666@gmail.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; }
Looks correct to me
Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com
Hello,
We have addressed the concurrency issue in the match driver interface at a higher level, as detailed in the [https://lore.kernel.org/all/20241112163041.40083-1-chenqiuji666@gmail.com/]. Due to the widespread nature of the issue, it is more appropriate to resolve it by adding a lock at the higher level.
If this patch is merged with the lock added at the lower level, it could potentially lead to a deadlock issue. Therefore, I kindly ask that you do not merge this patch. I sincerely apologize for the inconvenience caused and thank you for your understanding.
Regards, Qiu-ji Chen
On Fri, Oct 18, 2024 at 5:39 PM Suzuki K Poulose suzuki.poulose@arm.com wrote:
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@vger.kernel.org Signed-off-by: Qiu-ji Chen chenqiuji666@gmail.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;}
Looks correct to me
Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com
linux-stable-mirror@lists.linaro.org
-
Qiu-ji Chen -
Suzuki K Poulose