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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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>