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

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

[Dmitry Antipov]

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);
 	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);
 }
 
-- 
2.47.1

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

[Armin Wolf]

Am 16.12.24 um 14:24 schrieb Dmitry Antipov:

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

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

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

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

[Ilpo Järvinen]

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.

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

[Antipov, Dmitriy]

On Tue, 2024-12-17 at 16:47 +0200, Ilpo Järvinen wrote:


> When you use an "automated" tools to find "problems", you have to read 

Such a sentence would have much more effect if correctly rephrased.
For example: "Since I'm paying you, you have to [whatever]" etc.

Dmitry

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

On Thu, 19 Dec 2024, Antipov, Dmitriy wrote:
> On Tue, 2024-12-17 at 16:47 +0200, Ilpo Järvinen wrote:
> 
> > When you use an "automated" tools to find "problems", you have to read 
> 
> Such a sentence would have much more effect if correctly rephrased.
> For example: "Since I'm paying you, you have to [whatever]" etc.

No, that's not an optional thing when it comes to problems from automated 
tools. We've a special policy when it comes to contributions that 
originate from running analysis tools such as SVACE. For details, please 
see:

Documentation/process/researcher-guidelines.rst

In particular, please check the list after "Answer". It list among 
other things "How could the problem be reached on a running system?". To 
answer that, you need to understand the related code.

-- 
 i.