| 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
| ||
|
Message-ID: <20140906021757.GA9197@vmdeb7>
Date: Fri, 5 Sep 2014 19:17:57 -0700
From: Darren Hart <dvhart@...radead.org>
To: Paul Bolle <pebolle@...cali.nl>
Cc: Frans Klaver <fransklaver@...il.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Corentin Chary <corentin.chary@...il.com>,
Matthew Garrett <matthew.garrett@...ula.com>,
acpi4asus-user@...ts.sourceforge.net,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
Rafael Wysocki <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH] eeepc-laptop: remove possible use of uninitialized value
On Thu, Sep 04, 2014 at 09:08:08AM +0200, Paul Bolle wrote:
> On Thu, 2014-09-04 at 00:53 +0200, Frans Klaver wrote:
> > In store_sys_acpi, if count equals zero, or parse_arg()s sscanf call
> > fails, 'value' remains possibly uninitialized. In that case 'value'
> > shouldn't be used to produce the store_sys_acpi()s return value.
> >
> > Only test the return value of set_acpi() if we can actually call it.
> > Return rv otherwise.
> >
> > Signed-off-by: Frans Klaver <fransklaver@...il.com>
> > ---
> > drivers/platform/x86/eeepc-laptop.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index bd533c2..41f12ba 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -279,10 +279,10 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> > int rv, value;
> >
> > rv = parse_arg(buf, count, &value);
> > - if (rv > 0)
> > - value = set_acpi(eeepc, cm, value);
> > - if (value < 0)
> > - return -EIO;
> > + if (rv > 0) {
> > + if (set_acpi(eeepc, cm, value) < 0)
> > + return -EIO;
> > + }
> > return rv;
> > }
> >
>
> The warning that this code (currently) generated triggered me to submit
> https://lkml.org/lkml/2014/7/1/150 , which uses a different approach to
> get rid of it. I received no reactions so far. Here's that patch again:
Thanks for resending.
>
> ------------>8------------
> From: Paul Bolle <pebolle@...cali.nl>
> Subject: [PATCH] eeepc-laptop: simplify parse_arg()
>
> parse_arg() has three possible return values:
> -EINVAL if sscanf(), in short, fails;
> zero if "count" is zero; and
> "count" in all other cases
>
> But "count" will never be zero. See, parse_arg() is called by the
> various store functions. And the callchain of these functions starts
> with sysfs_kf_write(). And that function checks for a zero "count". So
> we can stop checking for a zero "count", drop the "count" argument
> entirely, and transform parse_arg() into a function that returns zero on
> success or a negative error. That, in turn, allows to make those store
> functions just return "count" on success. The net effect is that the
> code becomes a bit easier to understand.
>
Seems reasonable.
> A nice side effect is that this GCC warning is silenced too:
> drivers/platform/x86/eeepc-laptop.c: In function ‘store_sys_acpi’:
> drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> int rv, value;
>
> Which is, of course, the reason to have a look at parse_arg().
>
> Signed-off-by: Paul Bolle <pebolle@...cali.nl>
> ---
> drivers/platform/x86/eeepc-laptop.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index bd533c22be57..78515b850165 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -263,13 +263,11 @@ static int acpi_setter_handle(struct eeepc_laptop *eeepc, int cm,
> /*
> * Sys helpers
> */
> -static int parse_arg(const char *buf, unsigned long count, int *val)
> +static int parse_arg(const char *buf, int *val)
> {
> - if (!count)
> - return 0;
> if (sscanf(buf, "%i", val) != 1)
> return -EINVAL;
> - return count;
> + return 0;
> }
>
> static ssize_t store_sys_acpi(struct device *dev, int cm,
> @@ -278,12 +276,13 @@ static ssize_t store_sys_acpi(struct device *dev, int cm,
> struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
> int rv, value;
>
> - rv = parse_arg(buf, count, &value);
> - if (rv > 0)
> - value = set_acpi(eeepc, cm, value);
> + rv = parse_arg(buf, &value);
> + if (rv < 0)
> + return rv;
> + value = set_acpi(eeepc, cm, value);
> if (value < 0)
I suppose it's harmless, but it would be more explicit to reuse rv here instead
of value.
> return -EIO;
And as with Frans' version, I suggest propogating the error. We're talking about
a missing/invalid ACPI control method name here, ENODEV seems approprirate.
Rafael, do you have a strong preference about what to return in such an event?
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists