Re: [PATCH 2/3] media: sunxi: Add support for the D1 CSIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] pm: crash in IRQ handler due to missing pm_runtime_get_if_active()
- [High] media: UAF of sun20i_csi structure on device unbind
- [High] media: UAF of csi->source when sensor is unbound while streaming
- [Medium] pm: hardware left powered on after driver removal due to async pm_runtime_put
- [Medium] media: potential NULL dereference in sun20i_csi_bridge_enum_mbus_code()
--

commit 48923ca6acd2a7bc065ae49a8c19a8a6d69a0531
Author: Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog>

    media: sunxi: Add support for the D1 CSIC
    
    Add a driver for the parallel input of the D1 CSIC and its first DMA
    engine. The parser is a bridge subdev and the DMA engine a capture video
    device, set up through the media controller API.

> diff --git a/drivers/media/platform/sunxi/sun20i-csi/sun20i_csi.c b/drivers/media/platform/sunxi/sun20i-csi/sun20i_csi.c
> new file mode 100644
> index 0000000000000..eed29bc61348b
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun20i-csi/sun20i_csi.c
> @@ -0,0 +1,1357 @@

[ ... ]

> +static irqreturn_t sun20i_csi_irq(int irq, void *data)
> +{
> +	struct sun20i_csi *csi = data;
> +	u32 status;
> +
> +	status = sun20i_csi_read(csi, SUN20I_CSI_DMA_INT_STA_REG);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this code crash the system if an interrupt arrives while the device is
suspended?

The IRQ handler accesses hardware registers unconditionally without verifying
if the device is active. devm_request_irq() is called before
pm_runtime_enable() in sun20i_csi_probe(). If the bootloader left the
interrupt pending, the IRQ fires immediately before the device is powered on.
Additionally, since the IRQ is not disabled during runtime suspend, any
spurious interrupt while suspended triggers a synchronous external abort when
reading hardware registers while clocks are disabled.

Should this use pm_runtime_get_if_active() to verify the device is active
before accessing registers?

[ ... ]

> +static int sun20i_csi_bridge_init_state(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *state)
> +{
> +	*v4l2_subdev_state_get_format(state, SUN20I_CSI_PAD_SINK) =
> +		sun20i_csi_default_fmt;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Can this cause a NULL pointer dereference?

The format pointer for the unvalidated sink pad is dereferenced directly in
an initialization callback without framework validation guarantees.
v4l2_subdev_state_get_format() can return NULL.

> +	*v4l2_subdev_state_get_format(state, SUN20I_CSI_PAD_SOURCE) =
> +		sun20i_csi_default_fmt;
> +
> +	return 0;
> +}
> +
> +static int
> +sun20i_csi_bridge_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{

[ ... ]

> +	/* The source pad passes on the sink pad format. */
> +	if (code->pad == SUN20I_CSI_PAD_SOURCE) {
> +		const struct v4l2_mbus_framefmt *fmt;
> +
> +		if (code->index)
> +			return -EINVAL;
> +		fmt = v4l2_subdev_state_get_format(state, SUN20I_CSI_PAD_SINK);
> +		code->code = fmt->code;
                ^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Is it safe to assume fmt is non-NULL here?

In enum_mbus_code(), the core wrapper validates the state for the SOURCE pad.
The driver then unconditionally dereferences the format returned for the
opposite SINK pad. A check for one pad does not validate another.

[ ... ]

> +static int sun20i_csi_bridge_disable_streams(struct v4l2_subdev *sd,
> +					     struct v4l2_subdev_state *state,
> +					     u32 pad, u64 streams_mask)
> +{
> +	struct sun20i_csi *csi = sd_to_csi(sd);
> +	int ret;
> +
> +	ret = v4l2_subdev_disable_streams(csi->source, csi->source_pad,
> +					  BIT_ULL(0));
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this code dereference a freed pointer if the sensor is unbound while
streaming?

If the sensor subdevice is unbound (e.g., via sysfs) while the CSIC is
streaming, the async notifier core unbinds the sensor, but the CSIC driver
lacks an .unbind callback to clear the csi->source pointer. When streaming is
subsequently stopped by userspace, sun20i_csi_bridge_disable_streams()
dereferences the freed csi->source pointer.

[ ... ]

> +static void sun20i_csi_stop_streaming(struct vb2_queue *queue)
> +{
> +	struct sun20i_csi *csi = vb2_get_drv_priv(queue);
> +
> +	v4l2_subdev_disable_streams(&csi->subdev, SUN20I_CSI_PAD_SOURCE,
> +				    BIT_ULL(0));
> +	sun20i_csi_dma_stop(csi);
> +	video_device_pipeline_stop(&csi->vdev);
> +	sun20i_csi_return_buffers(csi, VB2_BUF_STATE_ERROR);
> +	pm_runtime_put(csi->dev);
        ^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Will this leave hardware powered on if the driver is unbound?

This queues an asynchronous suspend which is vulnerable to being cancelled by
driver removal. If the driver is unbound while streaming,
vb2_video_unregister_device() stops streaming synchronously, queuing an async
suspend via pm_runtime_put(). The remove function then finishes and devres
calls pm_runtime_disable(), which cancels the pending async suspend.

Should this use pm_runtime_put_sync() instead?

[ ... ]

> +static int sun20i_csi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sun20i_csi *csi;
> +	int ret;
> +
> +	csi = devm_kzalloc(dev, sizeof(*csi), GFP_KERNEL);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this allocation tie the video device lifetime to the driver binding
rather than the V4L2 file descriptors?

The structure embedding the video device is managed by devres. If userspace
holds /dev/videoX open when the driver unbinds, the remove function
completes and devres frees the structure. When userspace later closes the
file descriptor, v4l2_release() accesses the video_device embedded in the
freed structure, causing a Use-After-Free.

> +	if (!csi)
> +		return -ENOMEM;

[ ... ]

> +static void sun20i_csi_remove(struct platform_device *pdev)
> +{
> +	struct sun20i_csi *csi = platform_get_drvdata(pdev);
> +
> +	/* This stops streaming, which needs the whole pipeline. */
> +	vb2_video_unregister_device(&csi->vdev);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this code unregister the device without waiting for file descriptors to
close?

This synchronously stops streaming during driver unbind and unregisters the
device but does not wait for file descriptors to close. This allows the
devres cleanup to race with userspace closing the file, leading to the Use-
After-Free issue mentioned in sun20i_csi_probe().

Additionally, it immediately precedes the devres cleanup that cancels the
async put queued in sun20i_csi_stop_streaming().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930154835.402684-1-tien.nguyenminh@embeddedlinux.blog?part=2