Re: [PATCH v3 2/5] media: rkisp2: Add parameters output video node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The parameter buffer validation checks the userspace-mapped memory instead of the internal kernel copy, creating a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability that can lead to out-of-bounds memory access.
- [High] The first queued parameter buffer is permanently leaked every time streaming starts because it is dequeued but never completed.
- [Medium] Mutex and media entity resources are leaked on the error path if `video_register_device` fails.
--

commit 29257cb1b743c9d1fdc2e61085bac5b0b1b5d99d
Author: Paul Elder <paul.elder@ideasonboard.com>

media: rkisp2: Add parameters output video node
    
Implement support for setting parameters on the ISP by queueing
parameter buffers to rkisp2.

> diff --git a/drivers/media/platform/rockchip/rkisp2/rkisp2-params.c b/drivers/media/platform/rockchip/rkisp2/rkisp2-params.c
> index 0000000000000..07f57f26c7b0e
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rkisp2/rkisp2-params.c
[ ... ]
> +static int rkisp2_params_vb2_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct rkisp2_params *params = vb->vb2_queue->drv_priv;
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct rkisp2_params_buffer *params_buf = to_rkisp2_params_buffer(vbuf);
> +	struct v4l2_isp_params_buffer *cfg = vb2_plane_vaddr(&vbuf->vb2_buf, 0);
> +	size_t payload_size = vb2_get_plane_payload(vb, 0);
> +	int ret;
> +
> +	ret = v4l2_isp_params_validate_buffer_size(params->rkisp2->dev, vb,
> +						   v4l2_isp_buffer_size(RKISP2_PARAMS_MAX_SIZE));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the parameters buffer to the internal scratch buffer to avoid
> +	 * userspace modifying the buffer content while the driver processes it.
> +	 */
> +	memcpy(params_buf->cfg, cfg, payload_size);
> +
> +	return v4l2_isp_params_validate_buffer(params->rkisp2->dev, vb, cfg,
> +				rkisp2_params_block_types_info,
> +				ARRAY_SIZE(rkisp2_params_block_types_info));
> +}

[Severity: Critical]
Can this introduce a TOCTOU (Time-Of-Check to Time-Of-Use) regression?

The code correctly copies the userspace buffer to an internal scratch buffer
(params_buf->cfg) to prevent concurrent modification. However,
v4l2_isp_params_validate_buffer() is then called on the mutable userspace
pointer (cfg) rather than the safe internal copy (params_buf->cfg).

A malicious userspace program could submit invalid data, wait for the memcpy()
to finish, and then overwrite the shared memory with valid data before the
validation step completes.

Since the driver later uses the internal copy, it might end up consuming an
unvalidated parameter block type and using it to index rkisp2_params_handlers[],
which can lead to out-of-bounds memory access.

[ ... ]
> +void rkisp2_params_post_configure(struct rkisp2_params *params)
> +{
> +	struct rkisp2_params_buffer *buf;
> +
> +	spin_lock_irq(&params->buf_lock);
> +	buf = list_first_entry_or_null(&params->params,
> +				       struct rkisp2_params_buffer, queue);
> +	if (buf)
> +		list_del(&buf->queue);
> +	spin_unlock_irq(&params->buf_lock);
> +
> +	if (!buf)
> +		return;
> +
> +	rkisp2_params_configure(params, buf, RKISP2_PARAMS_CONFIG_PRIO_POST);
> +}

[Severity: High]
Is a buffer permanently leaked here when streaming starts?

When the ISP starts, the first queued parameter buffer is removed from the list
via list_del(&buf->queue) and passed to rkisp2_params_configure() with
RKISP2_PARAMS_CONFIG_PRIO_POST.

However, rkisp2_params_configure() explicitly checks for
RKISP2_PARAMS_CONFIG_PRIO_NONE before calling vb2_buffer_done():

static void rkisp2_params_configure(struct rkisp2_params *params,
...
	if (prio == RKISP2_PARAMS_CONFIG_PRIO_NONE)
		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
}

Since the buffer is no longer on the params->params list and vb2_buffer_done()
is skipped, it seems the buffer will never be returned to vb2. This could freeze
the pipeline or result in a WARN_ON during stop_streaming() due to left behind
active buffers.

[ ... ]
> +int rkisp2_params_register(struct rkisp2_device *rkisp2)
> +{
[ ... ]
> +	node->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> +	if (ret)
> +		goto err_media;
> +
> +	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> +	if (ret) {
> +		dev_err(rkisp2->dev,
> +			"failed to register %s, ret=%d\n", vdev->name, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +err_media:
> +	media_entity_cleanup(&vdev->entity);
> +	mutex_destroy(&node->vlock);
> +	return ret;
> +}

[Severity: Medium]
Are resources leaked on this error path if video_register_device() fails?

If video_register_device() returns an error, the code returns the error code
directly instead of jumping to the err_media cleanup block. This leaves the
media entity and node->vlock uncleaned, leaking these resources.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-rkisp2-v3-0-e91ff0a604d6@ideasonboard.com?part=2