On Wed, May 02, 2018 at 10:16:58AM +0100, Colin King wrote:
> From: Colin Ian King <colin.k...@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.k...@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
