| From ea354b6ddd6f09be29424f41fa75a3e637fea234 Mon Sep 17 00:00:00 2001 |
| From: Dan Carpenter <dan.carpenter@oracle.com> |
| Date: Thu, 21 Jan 2021 07:44:00 +0100 |
| Subject: media: zr364xx: fix memory leaks in probe() |
| |
| From: Dan Carpenter <dan.carpenter@oracle.com> |
| |
| commit ea354b6ddd6f09be29424f41fa75a3e637fea234 upstream. |
| |
| Syzbot discovered that the probe error handling doesn't clean up the |
| resources allocated in zr364xx_board_init(). There are several |
| related bugs in this code so I have re-written the error handling. |
| |
| 1) Introduce a new function zr364xx_board_uninit() which cleans up |
| the resources in zr364xx_board_init(). |
| 2) In zr364xx_board_init() if the call to zr364xx_start_readpipe() |
| fails then release the "cam->buffer.frame[i].lpvbits" memory |
| before returning. This way every function either allocates |
| everything successfully or it cleans up after itself. |
| 3) Re-write the probe function so that each failure path goto frees |
| the most recent allocation. That way we don't free anything |
| before it has been allocated and we can also verify that |
| everything is freed. |
| 4) Originally, in the probe function the "cam->v4l2_dev.release" |
| pointer was set to "zr364xx_release" near the start but I moved |
| that assignment to the end, after everything had succeeded. The |
| release function was never actually called during the probe cleanup |
| process, but with this change I wanted to make it clear that we |
| don't want to call zr364xx_release() until everything is |
| allocated successfully. |
| |
| Next I re-wrote the zr364xx_release() function. Ideally this would |
| have been a simple matter of copy and pasting the cleanup code from |
| probe and adding an additional call to video_unregister_device(). But |
| there are a couple quirks to note. |
| |
| 1) The probe function does not call videobuf_mmap_free() and I don't |
| know where the videobuf_mmap is allocated. I left the code as-is to |
| avoid introducing a bug in code I don't understand. |
| 2) The zr364xx_board_uninit() has a call to zr364xx_stop_readpipe() |
| which is a change from the original behavior with regards to |
| unloading the driver. Calling zr364xx_stop_readpipe() on a stopped |
| pipe is not a problem so this is safe and is potentially a bugfix. |
| |
| Reported-by: syzbot+b4d54814b339b5c6bbd4@syzkaller.appspotmail.com |
| Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> |
| Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> |
| Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/media/usb/zr364xx/zr364xx.c | 49 ++++++++++++++++++++++-------------- |
| 1 file changed, 31 insertions(+), 18 deletions(-) |
| |
| --- a/drivers/media/usb/zr364xx/zr364xx.c |
| +++ b/drivers/media/usb/zr364xx/zr364xx.c |
| @@ -1181,15 +1181,11 @@ out: |
| return err; |
| } |
| |
| -static void zr364xx_release(struct v4l2_device *v4l2_dev) |
| +static void zr364xx_board_uninit(struct zr364xx_camera *cam) |
| { |
| - struct zr364xx_camera *cam = |
| - container_of(v4l2_dev, struct zr364xx_camera, v4l2_dev); |
| unsigned long i; |
| |
| - v4l2_device_unregister(&cam->v4l2_dev); |
| - |
| - videobuf_mmap_free(&cam->vb_vidq); |
| + zr364xx_stop_readpipe(cam); |
| |
| /* release sys buffers */ |
| for (i = 0; i < FRAMES; i++) { |
| @@ -1200,9 +1196,19 @@ static void zr364xx_release(struct v4l2_ |
| cam->buffer.frame[i].lpvbits = NULL; |
| } |
| |
| - v4l2_ctrl_handler_free(&cam->ctrl_handler); |
| /* release transfer buffer */ |
| kfree(cam->pipe->transfer_buffer); |
| +} |
| + |
| +static void zr364xx_release(struct v4l2_device *v4l2_dev) |
| +{ |
| + struct zr364xx_camera *cam = |
| + container_of(v4l2_dev, struct zr364xx_camera, v4l2_dev); |
| + |
| + videobuf_mmap_free(&cam->vb_vidq); |
| + v4l2_ctrl_handler_free(&cam->ctrl_handler); |
| + zr364xx_board_uninit(cam); |
| + v4l2_device_unregister(&cam->v4l2_dev); |
| kfree(cam); |
| } |
| |
| @@ -1376,11 +1382,14 @@ static int zr364xx_board_init(struct zr3 |
| /* start read pipe */ |
| err = zr364xx_start_readpipe(cam); |
| if (err) |
| - goto err_free; |
| + goto err_free_frames; |
| |
| DBG(": board initialized\n"); |
| return 0; |
| |
| +err_free_frames: |
| + for (i = 0; i < FRAMES; i++) |
| + vfree(cam->buffer.frame[i].lpvbits); |
| err_free: |
| kfree(cam->pipe->transfer_buffer); |
| cam->pipe->transfer_buffer = NULL; |
| @@ -1409,12 +1418,10 @@ static int zr364xx_probe(struct usb_inte |
| if (!cam) |
| return -ENOMEM; |
| |
| - cam->v4l2_dev.release = zr364xx_release; |
| err = v4l2_device_register(&intf->dev, &cam->v4l2_dev); |
| if (err < 0) { |
| dev_err(&udev->dev, "couldn't register v4l2_device\n"); |
| - kfree(cam); |
| - return err; |
| + goto free_cam; |
| } |
| hdl = &cam->ctrl_handler; |
| v4l2_ctrl_handler_init(hdl, 1); |
| @@ -1423,7 +1430,7 @@ static int zr364xx_probe(struct usb_inte |
| if (hdl->error) { |
| err = hdl->error; |
| dev_err(&udev->dev, "couldn't register control\n"); |
| - goto fail; |
| + goto unregister; |
| } |
| /* save the init method used by this camera */ |
| cam->method = id->driver_info; |
| @@ -1496,7 +1503,7 @@ static int zr364xx_probe(struct usb_inte |
| if (!cam->read_endpoint) { |
| err = -ENOMEM; |
| dev_err(&intf->dev, "Could not find bulk-in endpoint\n"); |
| - goto fail; |
| + goto unregister; |
| } |
| |
| /* v4l */ |
| @@ -1507,10 +1514,11 @@ static int zr364xx_probe(struct usb_inte |
| |
| /* load zr364xx board specific */ |
| err = zr364xx_board_init(cam); |
| - if (!err) |
| - err = v4l2_ctrl_handler_setup(hdl); |
| if (err) |
| - goto fail; |
| + goto unregister; |
| + err = v4l2_ctrl_handler_setup(hdl); |
| + if (err) |
| + goto board_uninit; |
| |
| spin_lock_init(&cam->slock); |
| |
| @@ -1525,16 +1533,21 @@ static int zr364xx_probe(struct usb_inte |
| err = video_register_device(&cam->vdev, VFL_TYPE_VIDEO, -1); |
| if (err) { |
| dev_err(&udev->dev, "video_register_device failed\n"); |
| - goto fail; |
| + goto free_handler; |
| } |
| + cam->v4l2_dev.release = zr364xx_release; |
| |
| dev_info(&udev->dev, DRIVER_DESC " controlling device %s\n", |
| video_device_node_name(&cam->vdev)); |
| return 0; |
| |
| -fail: |
| +free_handler: |
| v4l2_ctrl_handler_free(hdl); |
| +board_uninit: |
| + zr364xx_board_uninit(cam); |
| +unregister: |
| v4l2_device_unregister(&cam->v4l2_dev); |
| +free_cam: |
| kfree(cam); |
| return err; |
| } |