Re: [PATCH v2 RESEND] media: hackrf: fix to not free memory after the device is registered in hackrf_probe()

[Jeongjun Park]

Hi Hans,

Hans Verkuil <hverkuil+cisco@xxxxxxxxxx> wrote:
>
> Hi Jeongjun Park,
>
> On 04/09/2025 07:42, Jeongjun Park wrote:
> > In hackrf driver, the following race condition occurs:
> > ```
> >               CPU0                                            CPU1
> > hackrf_probe()
> >   kzalloc(); // alloc hackrf_dev
> >   ....
> >   v4l2_device_register();
> >   ....
> >                                               open("/path/to/dev"); // open hackrf dev
> >                                               ....
> >   v4l2_device_unregister();
> >   ....
> >   kfree(); // free hackrf_dev
> >   ....
> >                                               ioctl(fd, ...);
> >                                                 v4l2_ioctl();
> >                                                   video_is_registered() // UAF!!
> >                                               ....
> >                                               close(fd);
> >                                                 v4l2_release() // UAF!!
> >                                                   hackrf_video_release()
> >                                                     kfree(); // DFB!!
> > ```
> >
> > When a V4L2 or video device is unregistered, the device node is removed so
> > new open() calls are blocked.
> >
> > However, file descriptors that are already open-and any in-flight I/O-do
> > not terminate immediately; they remain valid until the last reference is
> > dropped and the driver's release() is invoked.
> >
> > Therefore, freeing device memory on the error path after hackrf_probe()
> > has registered dev it will lead to a race to use-after-free vuln, since
> > those already-open handles haven't been released yet.
> >
> > And since release() free memory too, race to use-after-free and
> > double-free vuln occur.
> >
> > To prevent this, if device is registered from probe(), it should be
> > modified to free memory only through release() rather than calling
> > kfree() directly.
> >
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Reported-by: syzbot+6ffd76b5405c006a46b7@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=6ffd76b5405c006a46b7
> > Reported-by: syzbot+f1b20958f93d2d250727@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=f1b20958f93d2d250727
> > Fixes: 8bc4a9ed8504 ("[media] hackrf: add support for transmitter")
> > Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx>
> > ---
> > v2: Fix incorrect patch description style and CC stable mailing list
> > - Link to v1: https://lore.kernel.org/all/20250822142729.1156816-1-aha310510@xxxxxxxxx/
> > ---
> >  drivers/media/usb/hackrf/hackrf.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/media/usb/hackrf/hackrf.c b/drivers/media/usb/hackrf/hackrf.c
> > index 0b50de8775a3..d7a84422193d 100644
> > --- a/drivers/media/usb/hackrf/hackrf.c
> > +++ b/drivers/media/usb/hackrf/hackrf.c
> > @@ -1515,6 +1515,8 @@ static int hackrf_probe(struct usb_interface *intf,
> >       video_unregister_device(&dev->rx_vdev);
> >  err_v4l2_device_unregister:
> >       v4l2_device_unregister(&dev->v4l2_dev);
>
> Instead of v4l2_device_unregister() this should call v4l2_device_put().
> Otherwise the memory will never be freed since the v4l2_device refcount
> will never reach 0.

Oh, you're right.

Calling v4l2_device_unregister() instead of v4l2_device_put() causes a
memory leak here.

I'll write a v3 patch for hackrf and as102 and send it to you right away.

>
> > +     dev_dbg(&intf->dev, "failed=%d\n", ret);
>
> Drop this line, it doesn't add anything.
>
> Regards,
>
>         Hans
>
> > +     return ret;
> >  err_v4l2_ctrl_handler_free_tx:
> >       v4l2_ctrl_handler_free(&dev->tx_ctrl_handler);
> >  err_v4l2_ctrl_handler_free_rx:
> > --
> >
>

Regards,
Jeongjun Park