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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ddd60b0f-47a5-c93b-f055-d6900dfbd7de@linux.intel.com>
Date: Fri, 28 Mar 2025 15:53:19 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Arefev <arefev@...mel.ru>
cc: Corentin Chary <corentin.chary@...il.com>, 
    "Luke D. Jones" <luke@...nes.dev>, Hans de Goede <hdegoede@...hat.com>, 
    platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
    lvc-project@...uxtesting.org
Subject: Re: [PATCH] asus-laptop: Fix an uninitialized variable

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@...r.kernel.org
> > > Signed-off-by: Denis Arefev <arefev@...mel.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.