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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ