Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 23 Mar 2021 17:13:56 +0100
From:   Mauro Carvalho Chehab <mchehab@kernel.org>
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     hverkuil@xs4all.nl, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        syzbot+e7f4c64a4248a0340c37@syzkaller.appspotmail.com
Subject: Re: [PATCH] drivers/media/usb/gspca/stv06xx: fix memory leak
Message-ID: <20210323171356.4a613351@coco.lan>
In-Reply-To: <20210226233731.614553-1-paskripkin@gmail.com>
References: <20210226233731.614553-1-paskripkin@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Em Sat, 27 Feb 2021 02:37:31 +0300
Pavel Skripkin <paskripkin@gmail.com> escreveu:

> Syzbot reported memory leak in hdcs_probe_1x00()[1].
> hdcs_probe_1x00() allocates memory for struct hdcs, but if hdcs_init() fails in gspca_dev_probe2()
> this memory becomes leaked.
> 
> int gspca_dev_probe2(struct usb_interface *intf,
> 		const struct usb_device_id *id,
> 		const struct sd_desc *sd_desc,
> 		int dev_size,
> 		struct module *module)
> {
> ...
> 
> 	ret = sd_desc->config(gspca_dev, id);
> 	if (ret < 0)
> 		goto out;
> 	ret = sd_desc->init(gspca_dev);
> 	if (ret < 0)
> 		goto out;
> ...
> out:
> 	if (gspca_dev->input_dev)
> 		input_unregister_device(gspca_dev->input_dev);
> 	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> 	v4l2_device_unregister(&gspca_dev->v4l2_dev);
> 	kfree(gspca_dev->usb_buf);
> 	kfree(gspca_dev);
> 	return ret;
> }
> 
> Reported-by: syzbot+e7f4c64a4248a0340c37@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> Change-Id: Ia198671177ee346de61780813025110c7c491d7a
> ---
>  drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c
> index 5a47dcbf1c8e..24df13b33a02 100644
> --- a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c
> +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c
> @@ -485,7 +485,7 @@ static int hdcs_init(struct sd *sd)
>  					   stv_bridge_init[i][1]);
>  	}
>  	if (err < 0)
> -		return err;
> +		goto error;
>  
>  	/* sensor soft reset */
>  	hdcs_reset(sd);
> @@ -496,12 +496,12 @@ static int hdcs_init(struct sd *sd)
>  					     stv_sensor_init[i][1]);
>  	}
>  	if (err < 0)
> -		return err;
> +		goto error;
>  
>  	/* Enable continuous frame capture, bit 2: stop when frame complete */
>  	err = stv06xx_write_sensor(sd, HDCS_REG_CONFIG(sd), BIT(3));
>  	if (err < 0)
> -		return err;
> +		goto error;
>  
>  	/* Set PGA sample duration
>  	(was 0x7E for the STV602, but caused slow framerate with HDCS-1020) */
> @@ -512,9 +512,13 @@ static int hdcs_init(struct sd *sd)
>  		err = stv06xx_write_sensor(sd, HDCS_TCTRL,
>  				(HDCS_ADC_START_SIG_DUR << 5) | hdcs->psmp);
>  	if (err < 0)
> -		return err;
> +		goto error;
>  
>  	return hdcs_set_size(sd, hdcs->array.width, hdcs->array.height);
> +
> +error:
> +	kfree(hdcs);
> +	return err;
>  }

This doesn't seem the right fix here, as it is not the _init function
that allocates it. Also, when the device is disconnected, a memory leak
will happen.

I suspect that the right fix would be to move this:

	hdcs = kmalloc(sizeof(struct hdcs), GFP_KERNEL);
	if (!hdcs)
		return -ENOMEM;

To the main driver (stv06xx.c) - probably replacing it by kzalloc(),
and then handle the free code both both sd_probe() and sd_disconnect().


Thanks,
Mauro