On 1/24/20 3:13 PM, Dan Carpenter wrote: > Syzbot triggered a use after free in v5.5-rc6: > > BUG: KASAN: use-after-free in v4l2_release+0x2f1/0x390 drivers/media/v4l2-core/v4l2-dev.c:459 > > Allocated by task 94: > usbvision_alloc drivers/media/usb/usbvision/usbvision-video.c:1315 [inline] > usbvision_probe.cold+0x5c5/0x1f21 drivers/media/usb/usbvision/usbvision-video.c:1469 > > Freed by task 1913: > kfree+0xd5/0x300 mm/slub.c:3957 > usbvision_release+0x181/0x1c0 drivers/media/usb/usbvision/usbvision-video.c:1364 > usbvision_radio_close.cold+0x2b/0x74 drivers/media/usb/usbvision/usbvision-video.c:1130 > v4l2_release+0x2e7/0x390 drivers/media/v4l2-core/v4l2-dev.c:455 > > The problem is that the v4l2_release() calls usbvision_release() which > frees "usbvision" but v4l2_release() still wants to use > "usbvision->vdev". One solution is to make this devm_ allocated memory > so the memory isn't freed until later. devm_ allocated memory is freed after disconnect, so I doubt this will help, or at best it will just move the problem elsewhere. The right approach would be to use the release() callback from struct v4l2_device: that's called when the very last open filehandle is closed. But I'm not sure if it is worth the effort. The usbvision driver is a mess and personally I think it should be deprecated. Regards, Hans > > Reported-by: syzbot+75287f75e2fedd69d680@xxxxxxxxxxxxxxxxxxxxxxxxx > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > --- > I copied this idea from a different driver, but I haven't tested it. > I wanted to try the #syz fix command to see if it works. > > drivers/media/usb/usbvision/usbvision-video.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/media/usb/usbvision/usbvision-video.c b/drivers/media/usb/usbvision/usbvision-video.c > index 93d36aab824f..07b4763062c4 100644 > --- a/drivers/media/usb/usbvision/usbvision-video.c > +++ b/drivers/media/usb/usbvision/usbvision-video.c > @@ -1312,7 +1312,7 @@ static struct usb_usbvision *usbvision_alloc(struct usb_device *dev, > { > struct usb_usbvision *usbvision; > > - usbvision = kzalloc(sizeof(*usbvision), GFP_KERNEL); > + usbvision = devm_kzalloc(&dev->dev, sizeof(*usbvision), GFP_KERNEL); > if (!usbvision) > return NULL; > > @@ -1336,7 +1336,6 @@ static struct usb_usbvision *usbvision_alloc(struct usb_device *dev, > v4l2_ctrl_handler_free(&usbvision->hdl); > v4l2_device_unregister(&usbvision->v4l2_dev); > err_free: > - kfree(usbvision); > return NULL; > } > > @@ -1361,7 +1360,6 @@ static void usbvision_release(struct usb_usbvision *usbvision) > > v4l2_ctrl_handler_free(&usbvision->hdl); > v4l2_device_unregister(&usbvision->v4l2_dev); > - kfree(usbvision); > > PDEBUG(DBG_PROBE, "success"); > } >