amba: Fix atomicity violation in amba_match()

[PATCH] amba: Fix atomicity violation in amba_match()

Posted by Qiu-ji Chen 1 month, 1 week ago

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;
 }
-- 
2.34.1

Re: [PATCH] amba: Fix atomicity violation in amba_match()

Posted by Suzuki K Poulose 1 month, 1 week ago

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>

Re: [PATCH] amba: Fix atomicity violation in amba_match()

Posted by Qiu-ji Chen 1 week, 6 days ago

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