Re: [PATCH] media: usb: pvrusb2: fix slab-use-after-free in pvr2_v4l2_dev_init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix: Use-After-Free on video_register_device error path.
- [Medium] Memory leak when video_register_device fails early.
--

commit e9309ec9cc3bd0c7e640b415aa2e2d4a14232789
Author: Pei Xiao <xiaopei01@kylinos.cn>

media: usb: pvrusb2: fix slab-use-after-free in pvr2_v4l2_dev_init

This commit removes a redundant second video_register_device() call to
prevent a slab-use-after-free when the first registration attempt fails.

Link: https://lore.kernel.org/lkml/6C2D160B-37DD-40F0-B8A2-089B8CAACB58@gmail.com/

> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> index 101b2e9fbaab7..f9df813ca09b7 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> @@ -1215,13 +1215,10 @@ static void pvr2_v4l2_dev_init(struct pvr2_v4l2_dev *dip,
>  		mindevnum = nr_ptr[unit_number];
>  	}
>  	pvr2_hdw_set_v4l2_dev(hdw, &dip->devbase);
> -	if ((video_register_device(&dip->devbase,
> -				   dip->v4l_type, mindevnum) < 0) &&
> -	    (video_register_device(&dip->devbase,
> -				   dip->v4l_type, -1) < 0)) {
> +	if (video_register_device(&dip->devbase,
> +				   dip->v4l_type, mindevnum) < 0)
>  		pr_err(KBUILD_MODNAME
>  			": Failed to register pvrusb2 v4l device\n");

[Severity: High]
This is a pre-existing issue, but does execution fall through here if
video_register_device() fails?

If video_register_device() fails during device_register(), the V4L2 core
drops the device reference via put_device(). This triggers the driver's
pvr2_video_device_release() callback, freeing the dip structure.

Since there is no early return on error, execution continues and immediately
dereferences the freed dip object in the following pr_info() call and in
pvr2_hdw_v4l_store_minor_number().

Additionally, since pvr2_v4l2_dev_init() returns void, the caller
pvr2_v4l2_create() assumes success and retains the dangling pointer:

drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:pvr2_v4l2_create() {
    ...
    vp->dev_video = kzalloc_obj(*vp->dev_video);
    if (!vp->dev_video) goto fail;
    pvr2_v4l2_dev_init(vp->dev_video, vp, VFL_TYPE_VIDEO);
    ...
}

This leads to a secondary use-after-free when pvr2_v4l2_dev_destroy() accesses
vp->dev_video during device teardown.

Could we add an early return after logging the error?

[Severity: Medium]
This is also a pre-existing issue, but what happens if
video_register_device() fails early before device_register() is called?

In an early failure case (such as if cdev_alloc fails), the V4L2 core returns
an error without calling put_device(), meaning the dip structure is not freed
by the core.

Because pvr2_v4l2_dev_init() lacks error handling and falls through, it does
not explicitly free dip, and vp->dev_video retains the valid but unregistered
pointer.

During teardown, pvr2_v4l2_dev_destroy() passes this pointer to
video_unregister_device(), which checks if the device is registered:

drivers/media/v4l2-core/v4l2-dev.c:video_unregister_device() {
    ...
    /* Check if vdev was ever registered at all */
    if (!vdev || !video_is_registered(vdev))
        return;
    ...
}

Since the flag was never set, video_unregister_device() returns immediately
without unregistering or dropping the reference. Thus,
pvr2_video_device_release() is never invoked, permanently leaking the dip
memory allocation.

Would explicitly freeing the dip structure on early failure resolve this memory
leak?

>  
>  	pr_info("pvrusb2: registered device %s [%s]\n",
>  	       video_device_node_name(&dip->devbase),
>  	       pvr2_config_get_name(dip->config));
>  
>  	pvr2_hdw_v4l_store_minor_number(hdw,
>  					dip->minor_type,dip->devbase.minor);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_B85DFB2210B877A85021C1BC064E70E61C09@qq.com?part=1