* [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store()
@ 2015-07-19 19:43 Christian Engelmayer
2015-07-20 14:50 ` Gwendal Grignou
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Christian Engelmayer @ 2015-07-19 19:43 UTC (permalink / raw)
To: olof
Cc: lee.jones, gwendal, wfrichar, smbarber, linux-kernel,
Christian Engelmayer
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@gmx.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;
+ if (msg->result != EC_RES_SUCCESS)
goto exit;
- }
i = 0;
ok = 1;
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store()
2015-07-19 19:43 [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store() Christian Engelmayer
@ 2015-07-20 14:50 ` Gwendal Grignou
2015-07-20 20:57 ` Christian Engelmayer
2015-07-23 15:19 ` Lee Jones
2015-08-21 17:11 ` Olof Johansson
2 siblings, 1 reply; 5+ messages in thread
From: Gwendal Grignou @ 2015-07-20 14:50 UTC (permalink / raw)
To: Christian Engelmayer
Cc: Olof Johansson, Lee Jones, Gwendal Grignou, Bill Richardson,
Stephen Barber, Linux Kernel
On Sun, Jul 19, 2015 at 12:43 PM, Christian Engelmayer <cengelma@gmx.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@gmx.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.
> + if (msg->result != EC_RES_SUCCESS)
> goto exit;
> - }
>
> i = 0;
> ok = 1;
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store()
2015-07-20 14:50 ` Gwendal Grignou
@ 2015-07-20 20:57 ` Christian Engelmayer
0 siblings, 0 replies; 5+ messages in thread
From: Christian Engelmayer @ 2015-07-20 20:57 UTC (permalink / raw)
To: Gwendal Grignou
Cc: Olof Johansson, Lee Jones, Bill Richardson, Stephen Barber,
Linux Kernel
On Mon, 20 Jul 2015 07:50:36 -0700, Gwendal Grignou <gwendal@chromium.org> wrote:
> On Sun, Jul 19, 2015 at 12:43 PM, Christian Engelmayer <cengelma@gmx.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@gmx.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
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store()
2015-07-19 19:43 [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store() Christian Engelmayer
2015-07-20 14:50 ` Gwendal Grignou
@ 2015-07-23 15:19 ` Lee Jones
2015-08-21 17:11 ` Olof Johansson
2 siblings, 0 replies; 5+ messages in thread
From: Lee Jones @ 2015-07-23 15:19 UTC (permalink / raw)
To: Christian Engelmayer; +Cc: olof, gwendal, wfrichar, smbarber, linux-kernel
On Sun, 19 Jul 2015, Christian Engelmayer 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@gmx.at>
> ---
> Compile tested only. Applies against linux-next.
> ---
> drivers/platform/chrome/cros_ec_lightbar.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
This is not an MFD patch.
> 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;
> + if (msg->result != EC_RES_SUCCESS)
> goto exit;
> - }
>
> i = 0;
> ok = 1;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store()
2015-07-19 19:43 [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store() Christian Engelmayer
2015-07-20 14:50 ` Gwendal Grignou
2015-07-23 15:19 ` Lee Jones
@ 2015-08-21 17:11 ` Olof Johansson
2 siblings, 0 replies; 5+ messages in thread
From: Olof Johansson @ 2015-08-21 17:11 UTC (permalink / raw)
To: Christian Engelmayer; +Cc: lee.jones, gwendal, wfrichar, smbarber, linux-kernel
On Sun, Jul 19, 2015 at 09:43:02PM +0200, Christian Engelmayer 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@gmx.at>
> ---
> Compile tested only. Applies against linux-next.
Applied with adjusted patch subject.
-Olof
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-08-21 17:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-19 19:43 [PATCH] mfd: cros_ec: Fix possible leak in led_rgb_store() Christian Engelmayer
2015-07-20 14:50 ` Gwendal Grignou
2015-07-20 20:57 ` Christian Engelmayer
2015-07-23 15:19 ` Lee Jones
2015-08-21 17:11 ` Olof Johansson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox