Re: [PATCH v11 18/19] media: platform: Add mali-c55 parameters video node

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Daniel,

Thanks for the update.

On Mon, Jul 14, 2025 at 04:06:44PM +0100, Daniel Scally wrote:
> +static int
> +mali_c55_params_validate_buffer(struct device *dev,
> +				const struct v4l2_params_buffer *buffer)
> +{
> +	/* Only v1 is supported at the moment. */
> +	if (buffer->version != MALI_C55_PARAM_BUFFER_V1) {
> +		dev_dbg(dev, "Unsupported extensible format version: %u\n",
> +			buffer->version);
> +		return -EINVAL;
> +	}

Is there anything else to validate here?

I guess nothing is done with the information other than the value is being
checked above, but if it had an effect on something, one would need to copy
the information to memory not accessible to the user.

> +
> +	return 0;
> +}
> +
> +static int mali_c55_params_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct mali_c55_params *params = vb2_get_drv_priv(vb->vb2_queue);
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct mali_c55_params_buf *buf = to_mali_c55_params_buf(vbuf);
> +	struct mali_c55 *mali_c55 = params->mali_c55;
> +	struct v4l2_params_buffer *config;
> +	int ret;
> +
> +	ret = v4l2_params_buffer_validate(
> +		mali_c55->dev, vb,
> +		v4l2_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE),
> +		mali_c55_params_validate_buffer);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the parameters buffer provided by userspace to the internal
> +	 * scratch buffer. This protects against the chance of userspace making
> +	 * changed to the buffer content whilst the driver processes it.
> +	 */
> +	config = vb2_plane_vaddr(vb, 0);
> +	memcpy(buf->config, config, v4l2_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
> +
> +	return v4l2_params_blocks_validate(mali_c55->dev, buf->config,
> +					   mali_c55_block_handlers,
> +					   ARRAY_SIZE(mali_c55_block_handlers),
> +					   NULL);
> +}
> +
> +static void mali_c55_params_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct mali_c55_params *params = vb2_get_drv_priv(vb->vb2_queue);
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct mali_c55_params_buf *buf = to_mali_c55_params_buf(vbuf);
> +
> +	spin_lock(&params->buffers.lock);
> +	list_add_tail(&buf->queue, &params->buffers.queue);
> +	spin_unlock(&params->buffers.lock);
> +}
> +
> +static void mali_c55_params_return_buffers(struct mali_c55_params *params,
> +					   enum vb2_buffer_state state)
> +{
> +	struct mali_c55_params_buf *buf, *tmp;
> +
> +	guard(spinlock)(&params->buffers.lock);
> +
> +	list_for_each_entry_safe(buf, tmp, &params->buffers.queue, queue) {
> +		list_del(&buf->queue);
> +		vb2_buffer_done(&buf->vb.vb2_buf, state);
> +	}
> +}
> +
> +static int mali_c55_params_start_streaming(struct vb2_queue *q,
> +					   unsigned int count)
> +{
> +	struct mali_c55_params *params = vb2_get_drv_priv(q);
> +	struct mali_c55 *mali_c55 = params->mali_c55;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(mali_c55->dev);
> +	if (ret)
> +		goto err_return_buffers;
> +
> +	ret = video_device_pipeline_alloc_start(&params->vdev);
> +	if (ret)
> +		goto err_pm_put;
> +
> +	ret = video_device_pipeline_started(&params->vdev);
> +	if (ret < 0)
> +		goto err_stop_pipeline;
> +
> +	return 0;
> +
> +err_stop_pipeline:
> +	video_device_pipeline_stop(&params->vdev);
> +err_pm_put:
> +	pm_runtime_put(mali_c55->dev);
> +err_return_buffers:
> +	mali_c55_params_return_buffers(params, VB2_BUF_STATE_QUEUED);
> +
> +	return ret;
> +}
> +
> +static void mali_c55_params_stop_streaming(struct vb2_queue *q)
> +{
> +	struct mali_c55_params *params = vb2_get_drv_priv(q);
> +	struct media_pipeline *pipe;
> +
> +	pipe = video_device_pipeline(&params->vdev);
> +	if (mali_c55_pipeline_ready(pipe))
> +		media_pipeline_stopped(pipe);
> +
> +	video_device_pipeline_stop(&params->vdev);
> +	mali_c55_params_return_buffers(params, VB2_BUF_STATE_ERROR);
> +}
> +
> +static const struct vb2_ops mali_c55_params_vb2_ops = {
> +	.queue_setup = mali_c55_params_queue_setup,
> +	.buf_init = mali_c55_params_buf_init,
> +	.buf_cleanup = mali_c55_params_buf_cleanup,
> +	.buf_queue = mali_c55_params_buf_queue,
> +	.buf_prepare = mali_c55_params_buf_prepare,
> +	.wait_prepare = vb2_ops_wait_prepare,
> +	.wait_finish = vb2_ops_wait_finish,
> +	.start_streaming = mali_c55_params_start_streaming,
> +	.stop_streaming = mali_c55_params_stop_streaming,
> +};
> +
> +void mali_c55_params_write_config(struct mali_c55 *mali_c55)
> +{
> +	struct mali_c55_params *params = &mali_c55->params;
> +	struct v4l2_params_buffer *config;
> +	struct mali_c55_params_buf *buf;
> +	size_t block_offset = 0;
> +	size_t max_offset;
> +
> +	spin_lock(&params->buffers.lock);
> +
> +	buf = list_first_entry_or_null(&params->buffers.queue,
> +				       struct mali_c55_params_buf, queue);
> +	if (buf)
> +		list_del(&buf->queue);
> +	spin_unlock(&params->buffers.lock);
> +
> +	if (!buf)
> +		return;
> +
> +	buf->vb.sequence = mali_c55->isp.frame_sequence;
> +	config = buf->config;
> +
> +	max_offset = config->data_size - sizeof(struct v4l2_params_block_header);
> +
> +	/*
> +	 * Walk the list of parameter blocks and process them. No validation is
> +	 * done here, as the contents of the config buffer are already checked
> +	 * when the buffer is queued.
> +	 */
> +	while (block_offset < max_offset) {
> +		const struct v4l2_params_handler *block_handler;
> +		union mali_c55_params_block block;
> +
> +		block.data = &config->data[block_offset];
> +
> +		/* We checked the array index already in .buf_queue() */

Not a lot seems to be done there in terms of validation.

Even if that had been done, you can't trust the buffer contents as it
remains mapped to the userspace.

> +		block_handler = &mali_c55_block_handlers[block.header->type];
> +		block_handler->handler(mali_c55, block.header);
> +
> +		block_offset += block.header->size;
> +	}
> +
> +	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +}

-- 
Kind regards,

Sakari Ailus