| Message ID | 20180502091658.17110-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 02, 2018 at 10:16:58AM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > There are memory leaks of params; when copy_to_user fails and also > the exit via the label 'error'. Fix this by kfree'ing params in > error exit path and jumping to this on the copy_to_user failure path. > > Detected by CoverityScan, CID#1467966 ("Resource leak") > > Fixes: da43b6ccadcf ("[media] davinci: vpfe: dm365: add IPIPE support for media controller driver") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > index 95942768639c..3e67ee6e92f9 100644 > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > @@ -1252,12 +1252,12 @@ static const struct ipipe_module_if ipipe_modules[VPFE_IPIPE_MAX_MODULES] = { > static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) > { > struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd); > + struct ipipe_module_params *params; > unsigned int i; > int rval = 0; > > for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) { > const struct ipipe_module_if *module_if; > - struct ipipe_module_params *params; > void *from, *to; > size_t size; > > @@ -1275,7 +1275,7 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) > if (to && from && size) { ^^ This "to" check is wrong. Say "params" is NULL and module_if->param_offset is non-zero then "to" is a bogus pointer. We should just test "params" and give up the first time an allocation fails. > if (copy_from_user(to, (void __user *)from, size)) { > rval = -EFAULT; > - break; > + goto error; > } > rval = module_if->set(ipipe, to); > if (rval) > @@ -1287,7 +1287,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) > } > kfree(params); > } > + return rval; Doing a "return 0;" is more readable than "return rval;". regards, dan carpenter
diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index 95942768639c..3e67ee6e92f9 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c @@ -1252,12 +1252,12 @@ static const struct ipipe_module_if ipipe_modules[VPFE_IPIPE_MAX_MODULES] = { static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) { struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd); + struct ipipe_module_params *params; unsigned int i; int rval = 0; for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) { const struct ipipe_module_if *module_if; - struct ipipe_module_params *params; void *from, *to; size_t size; @@ -1275,7 +1275,7 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) if (to && from && size) { if (copy_from_user(to, (void __user *)from, size)) { rval = -EFAULT; - break; + goto error; } rval = module_if->set(ipipe, to); if (rval) @@ -1287,7 +1287,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) } kfree(params); } + return rval; error: + kfree(params); return rval; }