Re: [PATCH] platform/x86: acer-wmi: fix fan mode setup in WMID_gaming_set_fan_mode()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 16 Dec 2024, Dmitry Antipov wrote:

> In 'WMID_gaming_set_fan_mode()', most likely the (whether CPU or
> GPU or even total) fan count is not larger than 31. But still
> cast everyting to 'u64' just to be sure that there is no integer
> overflow when performing left shifts. Compile tested only.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/platform/x86/acer-wmi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index d09baa3d3d90..9be6176c0076 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1504,17 +1504,17 @@ static void WMID_gaming_set_fan_mode(u8 fan_mode)
>  	int i;
>  
>  	if (quirks->cpu_fans > 0)
> -		gpu_fan_config2 |= 1;
> +		gpu_fan_config2 |= 1ULL;
>  	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> -		gpu_fan_config2 |= 1 << (i + 1);
> +		gpu_fan_config2 |= 1ULL << (i + 1);
>  	for (i = 0; i < quirks->gpu_fans; ++i)
> -		gpu_fan_config2 |= 1 << (i + 3);
> +		gpu_fan_config2 |= 1ULL << (i + 3);

Now this change doesn't make much sense. You assumed that fan counts can 
be large which I find highly suspicious to begin with. Reading the code
easily reveals neither is never > 1!??!

But lets entartain the idea those counts could be large...

What about bit collisions if cpu_fans + gpu_fans > 2?

>  	if (quirks->cpu_fans > 0)
>  		gpu_fan_config1 |= fan_mode;
>  	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> -		gpu_fan_config1 |= fan_mode << (2 * i + 2);
> +		gpu_fan_config1 |= (u64)fan_mode << (2 * i + 2);
>  	for (i = 0; i < quirks->gpu_fans; ++i)
> -		gpu_fan_config1 |= fan_mode << (2 * i + 6);
> +		gpu_fan_config1 |= (u64)fan_mode << (2 * i + 6);
>  	WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);

This line tells us gpu_fan_config2 can only be up to a GENMASK(15, 0) 
field so if the type overflow problem would be real, there would be much 
bigger problems with this code than what this patch is trying to "fix".

When you use an "automated" tools to find "problems", you have to read and 
understand _all surrounding and related code_ before submitting patches 
like this. You could have easily seen that those counts are never larger 
than 1 and that the patch is not a real fix. Please keep that in mind 
before sending more fixes originating from automated tools.


(And yes, this code should be converted to use FIELD_PREP() and GENMASK(),
and the fan auto/turbo modes named with defines, etc.).


-- 
 i.