| Message ID | 20241216132400.302003-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | platform/x86: acer-wmi: fix fan mode setup in WMID_gaming_set_fan_mode() | expand |
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); > } >
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.).
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
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.
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); }
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(-)