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]
Date:	Mon, 20 Jul 2015 22:57:53 +0200
From:	Christian Engelmayer <cengelma@....at>
To:	Gwendal Grignou <gwendal@...omium.org>
Cc:	Olof Johansson <olof@...om.net>, Lee Jones <lee.jones@...aro.org>,
	Bill Richardson <wfrichar@...omium.org>,
	Stephen Barber <smbarber@...omium.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store()

On Mon, 20 Jul 2015 07:50:36 -0700, Gwendal Grignou <gwendal@...omium.org> wrote:
> On Sun, Jul 19, 2015 at 12:43 PM, Christian Engelmayer <cengelma@....at> wrote:
> > Function led_rgb_store() contains some direct returns in error cases that
> > leak the already allocated cros_ec_command message structure. Make sure
> > that 'msg' is freed in all exit paths. Detected by Coverity CID 1309666.
> >
> > Signed-off-by: Christian Engelmayer <cengelma@....at>
> > ---
> > Compile tested only. Applies against linux-next.
> > ---
> >  drivers/platform/chrome/cros_ec_lightbar.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> > index 144e09df9b84..4e598c11e8a4 100644
> > --- a/drivers/platform/chrome/cros_ec_lightbar.c
> > +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> > @@ -252,7 +252,7 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
> >
> >                 ret = sscanf(buf, "%i", &val[i++]);
> >                 if (ret == 0)
> > -                       return -EINVAL;
> > +                       goto exit;
> >
> >                 if (i == 4) {
> >                         param = (struct ec_params_lightbar *)msg->data;
> > @@ -268,17 +268,15 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
> >                         if ((j++ % 4) == 0) {
> >                                 ret = lb_throttle();
> >                                 if (ret)
> > -                                       return ret;
> > +                                       goto exit;
> >                         }
> >
> >                         ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
> >                         if (ret < 0)
> >                                 goto exit;
> >
> > -                       if (msg->result != EC_RES_SUCCESS) {
> > -                               ret = -EINVAL;
> ret = -EINVAL is necessary to indicate the command did not succeed:
> the command was successfully sent to the EC, and the response was
> received, but the EC failed the command internally.

That's the code pattern seen in this module, however, in that case setting
'ret' seems superfluous and potentially misleading, as the functions exit
code is written differently:

    exit:
            kfree(msg);
            return (ok && i == 0) ? count : -EINVAL;

> > +                       if (msg->result != EC_RES_SUCCESS)
> >                                 goto exit;
> > -                       }
> >
> >                         i = 0;
> >                         ok = 1;
> > --
> > 1.9.1
> >

--
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