linux-kernel - Re: [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-prev] [day] [month] [year] [list]

Message-ID: <026951f9-05af-4ee4-85d2-30236292f7f8@arm.com>
Date: Fri, 18 Oct 2024 10:39:20 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Qiu-ji Chen <chenqiuji666@...il.com>, linux@...linux.org.uk,
 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,
 stable@...r.kernel.org
Subject: Re: [PATCH] amba: Fix atomicity violation in amba_match()

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@...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;
>   }


Looks correct to me

Reviewed-by: Suzuki K Poulose <suzuki.poulose@....com>