Re: [PATCH v3 01/10] media: synopsys: hdmirx: free the driver data when the last user is gone

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: hdmirx: Use-After-Free of resets array during devres cleanup
--

commit e1b26e1980a2a31fc0b4801e98e60468eba4a502
Author: Sascha Hauer <s.hauer@pengutronix.de>

media: synopsys: hdmirx: free the driver data when the last user is gone

The driver data is allocated with devm_kzalloc() and contains the
video_device, the vb2 queue and the control handler. devres frees it when
the device is unbound, but an open file descriptor on the video device
node keeps the video_device referenced beyond that point.

> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index 25f8ca0d6d946..bc541431638e7 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
[ ... ]
> @@ -2646,6 +2642,16 @@ static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,
>  	return 0;
>  }
>  
> +/* Runs when the last user of a device node is gone, possibly after remove() */
> +static void hdmirx_v4l2_release(struct v4l2_device *v4l2_dev)
> +{
> +	struct snps_hdmirx_dev *hdmirx_dev =
> +		container_of(v4l2_dev, struct snps_hdmirx_dev, v4l2_dev);
> +
> +	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
> +	kfree(hdmirx_dev);
        ^^^^

[Severity: High]
hdmirx_parse_dt() calls devm_reset_control_bulk_get_exclusive(), which
registers a devres action that stores the pointer to the inline array
hdmirx_dev->resets. 

When the last v4l2 user is gone, this kfree(hdmirx_dev) frees the driver
data structure synchronously. The driver core will later invoke
devres_release_all(), which calls devm_reset_control_bulk_release(). That
release function will attempt to iterate and put the resets using the
stored pointer, which now points into the already freed hdmirx_dev.

Does this code cause a use-after-free during devres cleanup?

> +}
> +
>  static int hdmirx_probe(struct platform_device *pdev)
>  {
>  	struct snps_hdmirx_dev *hdmirx_dev;
[ ... ]
> @@ -2771,10 +2780,16 @@ static int hdmirx_probe(struct platform_device *pdev)
>  	vb2_video_unregister_device(&hdmirx_dev->stream.vdev);
>  err_unreg_v4l2_dev:
>  	v4l2_device_unregister(&hdmirx_dev->v4l2_dev);
> -err_hdl:
> -	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
> +	hdmirx_disable(dev);
> +	v4l2_device_put(&hdmirx_dev->v4l2_dev);
> +
> +	return ret;
> +
>  err_pm:
>  	hdmirx_disable(dev);
> +	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
> +err_free:
> +	kfree(hdmirx_dev);
        ^^^^

[Severity: High]
If probe fails after hdmirx_parse_dt() has successfully called
devm_reset_control_bulk_get_exclusive(), hdmirx_dev is explicitly freed
here. Once probe returns, the driver core will run devres cleanup.

Does this error path result in the same use-after-free when accessing
hdmirx_dev->resets?

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-hdmirx-media-v3-0-71a11a2e3e76@pengutronix.de?part=1