| Message ID | X/WMfVDCsxRghKHH@mwanda (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: zr364xx: fix memory leaks in probe() | expand |
On Wed, Jan 06, 2021 at 01:10:05PM +0300, Dan Carpenter wrote: > 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. Dan: I recently sent in a patch for a similar problem in the gspca driver (commit e469d0b09a19 "media: gspca: Fix memory leak in probe"). It seems there may be similar issues in that driver: one single function call tries to undo an indeterminate number of initializations. I don't know enough about these subsystems to evaluate this. Can you take a look at it? Thank, Alan Stern
On Wed, Jan 06, 2021 at 11:45:50AM -0500, Alan Stern wrote: > On Wed, Jan 06, 2021 at 01:10:05PM +0300, Dan Carpenter wrote: > > 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. > > Dan: > > I recently sent in a patch for a similar problem in the gspca driver > (commit e469d0b09a19 "media: gspca: Fix memory leak in probe"). It > seems there may be similar issues in that driver: one single function > call tries to undo an indeterminate number of initializations. > > I don't know enough about these subsystems to evaluate this. Can you > take a look at it? > The probe error handling in gspca_dev_probe2() is fine now. All those functions are no-ops when they haven't been allocated/registered. regards, dan carpenter
Hi Dan, On 06/01/2021 11:10, Dan Carpenter wrote: > 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 several quirks to note. > > 1) The original code never called video_unregister_device(). In other > words, this is an additional bugfix. Not a bug, see below. > 2) 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. > 3) 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> > --- > drivers/media/usb/zr364xx/zr364xx.c | 50 ++++++++++++++++++----------- > 1 file changed, 32 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c > index 1e1c6b4d1874..62a232f995a7 100644 > --- a/drivers/media/usb/zr364xx/zr364xx.c > +++ b/drivers/media/usb/zr364xx/zr364xx.c > @@ -1181,15 +1181,11 @@ static int zr364xx_open(struct file *file) > 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,20 @@ static void zr364xx_release(struct v4l2_device *v4l2_dev) > 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); > + video_unregister_device(&cam->vdev); video_unregister_device() is called in the disconnect function, so calling it again here is wrong (it detects that the video device is already unregistered, so this effectively is a NOP). I have tested this patch with an actual zr364xx device without this line and it works fine. So you can add my: Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> for a v2 without this spurious video_unregister_device(). The v4l2_dev.release() callback is called when all video devices are unregistered and when all open filehandles to those video devices are closed, i.e. it is called when the very last user is gone. Regards, Hans > + v4l2_ctrl_handler_free(&cam->ctrl_handler); > + zr364xx_board_uninit(cam); > + v4l2_device_unregister(&cam->v4l2_dev); > kfree(cam); > } > > @@ -1376,11 +1383,14 @@ static int zr364xx_board_init(struct zr364xx_camera *cam) > /* 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 +1419,10 @@ static int zr364xx_probe(struct usb_interface *intf, > 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 +1431,7 @@ static int zr364xx_probe(struct usb_interface *intf, > 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 +1504,7 @@ static int zr364xx_probe(struct usb_interface *intf, > if (!cam->read_endpoint) { > err = -ENOMEM; > dev_err(&intf->dev, "Could not find bulk-in endpoint\n"); > - goto fail; > + goto unregister; > } > > /* v4l */ > @@ -1507,10 +1515,11 @@ static int zr364xx_probe(struct usb_interface *intf, > > /* 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 +1534,21 @@ static int zr364xx_probe(struct usb_interface *intf, > 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; > } >
On Wed, Jan 13, 2021 at 05:13:41PM +0100, Hans Verkuil wrote: > Hi Dan, > > On 06/01/2021 11:10, Dan Carpenter wrote: > > 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 several quirks to note. > > > > 1) The original code never called video_unregister_device(). In other > > words, this is an additional bugfix. > > Not a bug, see below. > Thanks for reviewing this. I will fix a send a v2. I should have seen that. The layering here is sort of confusing in a way... But not anything that needs to be dealt with immediately. regards, dan carpenter
diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c index 1e1c6b4d1874..62a232f995a7 100644 --- a/drivers/media/usb/zr364xx/zr364xx.c +++ b/drivers/media/usb/zr364xx/zr364xx.c @@ -1181,15 +1181,11 @@ static int zr364xx_open(struct file *file) 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,20 @@ static void zr364xx_release(struct v4l2_device *v4l2_dev) 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); + video_unregister_device(&cam->vdev); + v4l2_ctrl_handler_free(&cam->ctrl_handler); + zr364xx_board_uninit(cam); + v4l2_device_unregister(&cam->v4l2_dev); kfree(cam); } @@ -1376,11 +1383,14 @@ static int zr364xx_board_init(struct zr364xx_camera *cam) /* 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 +1419,10 @@ static int zr364xx_probe(struct usb_interface *intf, 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 +1431,7 @@ static int zr364xx_probe(struct usb_interface *intf, 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 +1504,7 @@ static int zr364xx_probe(struct usb_interface *intf, if (!cam->read_endpoint) { err = -ENOMEM; dev_err(&intf->dev, "Could not find bulk-in endpoint\n"); - goto fail; + goto unregister; } /* v4l */ @@ -1507,10 +1515,11 @@ static int zr364xx_probe(struct usb_interface *intf, /* 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 +1534,21 @@ static int zr364xx_probe(struct usb_interface *intf, 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; }
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 several quirks to note. 1) The original code never called video_unregister_device(). In other words, this is an additional bugfix. 2) 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. 3) 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> --- drivers/media/usb/zr364xx/zr364xx.c | 50 ++++++++++++++++++----------- 1 file changed, 32 insertions(+), 18 deletions(-)