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

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

Hi Dan,

On Wed, Jul 30, 2025 at 10:11:22PM +0100, Dan Scally wrote:
> Hi Sakari, thanks for the comments
> 
> On 30/07/2025 15:36, Sakari Ailus wrote:
> > 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?
> 
> 
> Not at this stage; the rest of the validation is done by Jacopo's new
> framework
> 
> > 
> > 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.
> 
> 
> Thats done later in .buf_prepare()

Ah, thanks; I missed that earlier.

> 
> > 
> > > +
> > > +	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.
> 
> Ah; a hangover of an earlier iteration before using the new v4l2-params
> helpers; the validation is done by those functions in .buf_prepare() now and
> the userspace buffer is memcopied to a scratch buffer.

Ack, I'll take a look at Jacopo's set and get back to this. It looks nice
overall.

-- 
Kind regards,

Sakari Ailus