[v1] asus-laptop: Fix an uninitialized variable

[PATCH] asus-laptop: Fix an uninitialized variable

Posted by Denis Arefev 8 months, 4 weeks ago

The value returned by the acpi_evaluate_integer() function is not
checked, but the result is not always successful, so an uninitialized
'val' variable may be used in calculations.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: b23910c2194e ("asus-laptop: Pegatron Lucid accelerometer")
Cc: stable@vger.kernel.org 
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 drivers/platform/x86/asus-laptop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
index d460dd194f19..b74b7d0eb6c2 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -427,7 +427,7 @@ static int asus_pega_lucid_set(struct asus_laptop *asus, int unit, bool enable)
 static int pega_acc_axis(struct asus_laptop *asus, int curr, char *method)
 {
 	int i, delta;
-	unsigned long long val;
+	unsigned long long val = PEGA_ACC_CLAMP;
 	for (i = 0; i < PEGA_ACC_RETRIES; i++) {
 		acpi_evaluate_integer(asus->handle, method, NULL, &val);
 
-- 
2.43.0

Re: [PATCH] asus-laptop: Fix an uninitialized variable

Posted by Ilpo Järvinen 8 months, 4 weeks ago

On Tue, 25 Mar 2025, Denis Arefev wrote:

> The value returned by the acpi_evaluate_integer() function is not
> checked, but the result is not always successful, so an uninitialized
> 'val' variable may be used in calculations.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: b23910c2194e ("asus-laptop: Pegatron Lucid accelerometer")
> Cc: stable@vger.kernel.org 
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>  drivers/platform/x86/asus-laptop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> index d460dd194f19..b74b7d0eb6c2 100644
> --- a/drivers/platform/x86/asus-laptop.c
> +++ b/drivers/platform/x86/asus-laptop.c
> @@ -427,7 +427,7 @@ static int asus_pega_lucid_set(struct asus_laptop *asus, int unit, bool enable)
>  static int pega_acc_axis(struct asus_laptop *asus, int curr, char *method)
>  {
>  	int i, delta;
> -	unsigned long long val;
> +	unsigned long long val = PEGA_ACC_CLAMP;
>  	for (i = 0; i < PEGA_ACC_RETRIES; i++) {
>  		acpi_evaluate_integer(asus->handle, method, NULL, &val);

Shouldn't you handle the error from acpi_evaluate_integer() properly 
instead?

-- 
 i.

Re: [PATCH] asus-laptop: Fix an uninitialized variable

Posted by Arefev 8 months, 3 weeks ago

25.03.2025 14:18, Ilpo Järvinen пишет:
> On Tue, 25 Mar 2025, Denis Arefev wrote:
>
>> The value returned by the acpi_evaluate_integer() function is not
>> checked, but the result is not always successful, so an uninitialized
>> 'val' variable may be used in calculations.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: b23910c2194e ("asus-laptop: Pegatron Lucid accelerometer")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Denis Arefev <arefev@swemel.ru>
>> ---
>>   drivers/platform/x86/asus-laptop.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
>> index d460dd194f19..b74b7d0eb6c2 100644
>> --- a/drivers/platform/x86/asus-laptop.c
>> +++ b/drivers/platform/x86/asus-laptop.c
>> @@ -427,7 +427,7 @@ static int asus_pega_lucid_set(struct asus_laptop *asus, int unit, bool enable)
>>   static int pega_acc_axis(struct asus_laptop *asus, int curr, char *method)
>>   {
>>   	int i, delta;
>> -	unsigned long long val;
>> +	unsigned long long val = PEGA_ACC_CLAMP;
>>   	for (i = 0; i < PEGA_ACC_RETRIES; i++) {
>>   		acpi_evaluate_integer(asus->handle, method, NULL, &val);
> Shouldn't you handle the error from acpi_evaluate_integer() properly
> instead?
>
Apparently, the developer realized that the output is very noisy and
therefore created an algorithm that will surely return a good result.

I did not check the return value, because if acpi_evaluate_integer()
cannot read the values of accelerometers, 'val' will remain
uninitialized and will be used in further calculations.

Re: [PATCH] asus-laptop: Fix an uninitialized variable

Posted by Ilpo Järvinen 8 months, 3 weeks ago

On Wed, 26 Mar 2025, Arefev wrote:

> 25.03.2025 14:18, Ilpo Järvinen пишет:
> > On Tue, 25 Mar 2025, Denis Arefev wrote:
> > 
> > > The value returned by the acpi_evaluate_integer() function is not
> > > checked, but the result is not always successful, so an uninitialized
> > > 'val' variable may be used in calculations.
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > > 
> > > Fixes: b23910c2194e ("asus-laptop: Pegatron Lucid accelerometer")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Denis Arefev <arefev@swemel.ru>
> > > ---
> > >   drivers/platform/x86/asus-laptop.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/asus-laptop.c
> > > b/drivers/platform/x86/asus-laptop.c
> > > index d460dd194f19..b74b7d0eb6c2 100644
> > > --- a/drivers/platform/x86/asus-laptop.c
> > > +++ b/drivers/platform/x86/asus-laptop.c
> > > @@ -427,7 +427,7 @@ static int asus_pega_lucid_set(struct asus_laptop
> > > *asus, int unit, bool enable)
> > >   static int pega_acc_axis(struct asus_laptop *asus, int curr, char
> > > *method)
> > >   {
> > >   	int i, delta;
> > > -	unsigned long long val;
> > > +	unsigned long long val = PEGA_ACC_CLAMP;
> > >   	for (i = 0; i < PEGA_ACC_RETRIES; i++) {
> > >   		acpi_evaluate_integer(asus->handle, method, NULL, &val);
> > Shouldn't you handle the error from acpi_evaluate_integer() properly
> > instead?
> > 
> Apparently, the developer realized that the output is very noisy and
> therefore created an algorithm that will surely return a good result.
> 
> I did not check the return value, because if acpi_evaluate_integer()
> cannot read the values of accelerometers, 'val' will remain
> uninitialized and will be used in further calculations.

But if ACPI doesn't provide a value, why should that clamp value be used 
instead? I'd tend to think curr would be more suitable "default".

But shouldn't the loop either use continue to retry or the function 
return curr right away if acpi_evaluate_integer() returns an error?

I just don't see how your patch improves situation here despite silencing 
the checker tool.

-- 
 i.

Re: [PATCH] asus-laptop: Fix an uninitialized variable

Posted by Denis Arefev 8 months, 2 weeks ago

> But if ACPI doesn't provide a value, why should that clamp value be used 
> instead? I'd tend to think curr would be more suitable "default".
> But shouldn't the loop either use continue to retry or the function 
> return curr right away if acpi_evaluate_integer() returns an error?

Hi Ilpo.

I'm sorry it took me so long to respond.

Yes, your comments are correct.
I hope the new patch will help to make the algorithm less noisy.

Regards Denis.