Re: [PATCH 8/8] media: rkisp1: Copy and validate parameters buffer

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Jun 12, 2024 at 03:28:05PM +0100, Daniel Scally wrote:
> Hi Jacopo. As mentioned in the last patch, I think that this could be
> squashed into 5/8, but a couple of comments below

I think it should be moved earlier, yes, probably even before the
introduction of extended parameters.

> On 05/06/2024 17:54, Jacopo Mondi wrote:
> > With the introduction of the extensible parameters format support in the
> > rkisp1-param.c module, the RkISP1 driver now configures the ISP blocks
> > by parsing the content of a data buffer of variable size provided by
> > userspace through the V4L2 meta-output interface using the MMAP memory
> > handling mode.
> >
> > As the parameters buffer is mapped in the userspace process memory,
> > applications have access to the buffer content while the driver
> > parses it.
> >
> > To prevent potential issues during the parameters buffer parsing and
> > processing in the driver, implement three vb2_ops to
> >
> > 1) allocate a scratch buffer in the driver private buffer structure
> > 2) validate the buffer content at VIDIOC_QBUF time
> > 3) copy the content of the user provided configuration parameters
> >     in the driver-private scratch buffer
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >   .../platform/rockchip/rkisp1/rkisp1-params.c  | 154 ++++++++++++++----
> >   1 file changed, 124 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 4adaf084ce6e..003239e14511 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -5,6 +5,8 @@
> >    * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> >    */
> >   
> > +#include <linux/string.h>
> > +
> >   #include <media/v4l2-common.h>
> >   #include <media/v4l2-event.h>
> >   #include <media/v4l2-ioctl.h>
> > @@ -1943,17 +1945,14 @@ static const struct rkisp1_ext_params_handler {
> >   };
> >   
> >   static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > -				      struct rkisp1_ext_params_cfg *cfg,
> > +				      struct rkisp1_params_buffer *buffer,
> >   				      u32 block_group_mask)
> >   {
> > +	struct rkisp1_ext_params_cfg *cfg = buffer->cfg;

Maybe do this in the callers to avoid changing the prototype of this
function and the other functions below.

> >   	size_t block_offset = 0;
> >   
> > -	if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) {
> > -		dev_dbg(params->rkisp1->dev,
> > -			"Invalid parameters buffer size %llu\n",
> > -			cfg->total_size);
> > -		return -EINVAL;
> > -	}
> > +	if (WARN_ON(!cfg))
> > +		return -ENOMEM;
> >   
> >   	/* Walk the list of parameter blocks and process them. */
> >   	while (block_offset < cfg->total_size) {
> > @@ -1965,25 +1964,13 @@ static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> >   		block_offset += block->size;
> >   
> >   		/*
> > -		 * Validate the block id and make sure the block group is in
> > -		 * the list of groups to configure.
> > +		 * Make sure the block group is in  the list of groups to
> > +		 * configure.
> >   		 */
> > -		if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) {
> > -			dev_dbg(params->rkisp1->dev,
> > -				"Invalid parameters block type\n");
> > -			return -EINVAL;
> > -		}
> > -
> >   		block_handler = &rkisp1_ext_params_handlers[block->type];
> >   		if (!(block_handler->group & block_group_mask))
> >   			continue;
> >   
> > -		if (block->size != block_handler->size) {
> > -			dev_dbg(params->rkisp1->dev,
> > -				"Invalid parameters block size\n");
> > -			return -EINVAL;
> > -		}
> > -
> >   		block_handler->handler(params, block);
> >   	}
> >   
> > @@ -1991,9 +1978,9 @@ static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> >   }
> >   
> >   static int rkisp1_ext_params_config(struct rkisp1_params *params,
> > -				    struct rkisp1_ext_params_cfg *cfg)
> > +				    struct rkisp1_params_buffer *buffer)
> >   {
> > -	return __rkisp1_ext_params_config(params, cfg,
> > +	return __rkisp1_ext_params_config(params, buffer,
> >   					  RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> >   					  RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC |
> >   					  RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> > @@ -2001,17 +1988,17 @@ static int rkisp1_ext_params_config(struct rkisp1_params *params,
> >   
> >   static int
> >   rkisp1_ext_params_other_meas_config(struct rkisp1_params *params,
> > -				    struct rkisp1_ext_params_cfg *cfg)
> > +				    struct rkisp1_params_buffer *buffer)
> >   {
> > -	return __rkisp1_ext_params_config(params, cfg,
> > +	return __rkisp1_ext_params_config(params, buffer,
> >   					  RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> >   					  RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> >   }
> >   
> >   static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params,
> > -					struct rkisp1_ext_params_cfg *cfg)
> > +					struct rkisp1_params_buffer *buffer)
> >   {
> > -	return __rkisp1_ext_params_config(params, cfg,
> > +	return __rkisp1_ext_params_config(params, buffer,
> >   					  RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC);
> >   }
> >   
> > @@ -2057,7 +2044,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> >   		rkisp1_isp_isr_lsc_config(params, cfg);
> >   		rkisp1_isp_isr_meas_config(params, cfg);
> >   	} else {
> > -		ret = rkisp1_ext_params_config(params, cfg);
> > +		ret = rkisp1_ext_params_config(params, buf);
> >   	}
> >   
> >   	if (ret)
> > @@ -2168,7 +2155,7 @@ int rkisp1_params_pre_configure(struct rkisp1_params *params,
> >   		rkisp1_isp_isr_other_config(params, cfg);
> >   		rkisp1_isp_isr_meas_config(params, cfg);
> >   	} else {
> > -		ret = rkisp1_ext_params_other_meas_config(params, cfg);
> > +		ret = rkisp1_ext_params_other_meas_config(params, buf);
> >   	}
> >   
> >   	if (ret) {
> > @@ -2215,7 +2202,7 @@ int rkisp1_params_post_configure(struct rkisp1_params *params)
> >   	if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS)
> >   		rkisp1_isp_isr_lsc_config(params, cfg);
> >   	else
> > -		ret = rkisp1_ext_params_lsc_config(params, cfg);
> > +		ret = rkisp1_ext_params_lsc_config(params, buf);
> >   
> >   	if (ret)
> >   		goto complete_and_unlock;
> > @@ -2407,6 +2394,110 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
> >   	return 0;
> >   }
> >   
> > +static int rkisp1_params_vb2_buf_init(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct rkisp1_params_buffer *params_buf =
> > +		container_of(vbuf, struct rkisp1_params_buffer, vb);

A to_rkisp1_params_buffer() inline function in the previous patch would
be nice.

> > +	struct rkisp1_params *params = vb->vb2_queue->drv_priv;
> > +	size_t buf_size = params->metafmt.buffersize;
> > +
> > +	if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > +		params_buf->cfg = NULL;
> > +		return 0;
> > +	}

The problem is not restricted to the extensible format, how about doing
the same for the legacy format ?

> > +
> > +	params_buf->cfg = kvmalloc(buf_size, GFP_KERNEL);
> > +	if (IS_ERR_OR_NULL(params_buf->cfg))

Can kvmalloc() return an error pointer ?

> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +
> > +static void rkisp1_params_vb2_buf_cleanup(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct rkisp1_params_buffer *params_buf =
> > +		container_of(vbuf, struct rkisp1_params_buffer, vb);
> > +
> > +	kvfree(params_buf->cfg);

	params_buf->cfg = NULL;

to be safe.

> > +}
> > +
> > +static int rkisp1_params_validate_ext_params(struct rkisp1_params *params,
> > +					     struct rkisp1_ext_params_cfg *cfg)
> > +{
> > +	size_t block_offset = 0;
> > +
> > +	if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) {
> > +		dev_dbg(params->rkisp1->dev,
> > +			"Invalid parameters buffer size %llu\n",
> > +			cfg->total_size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Walk the list of parameter blocks and validate them. */
> > +	while (block_offset < cfg->total_size) {
> > +		const struct rkisp1_ext_params_handler *hdlr;
> > +		struct rkisp1_ext_params_block_header *block;
> > +
> > +		block = (struct rkisp1_ext_params_block_header *)
> > +			&cfg->data[block_offset];
> > +		block_offset += block->size;

Move this line to the end of the loop to avoid block_offset ever
pointing beyond the end of the buffer.

> > +
> > +		if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) {
> > +			dev_dbg(params->rkisp1->dev,
> > +				"Invalid parameters block type\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		hdlr = &rkisp1_ext_params_handlers[block->type];
> > +		if (hdlr->group != RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS &&
> > +		    hdlr->group != RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC &&
> > +		    hdlr->group != RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS) {
> > +			dev_dbg(params->rkisp1->dev,
> > +				"Invalid parameters block group\n");
> > +			return -EINVAL;
> > +		}
>
> I think this check can probably be dropped; those values are from the
> kernel driver rather than userspace inputs.
>
> > +
> > +		if (block->size != hdlr->size) {
> > +			dev_dbg(params->rkisp1->dev,
> > +				"Invalid parameters block size\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rkisp1_params_vb2_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct rkisp1_params_buffer *params_buf =
> > +		container_of(vbuf, struct rkisp1_params_buffer, vb);
> > +	struct vb2_queue *vq = vb->vb2_queue;
> > +	struct rkisp1_params *params = vq->drv_priv;
> > +	struct rkisp1_ext_params_cfg *cfg =
> > +		vb2_plane_vaddr(&params_buf->vb.vb2_buf, 0);
> > +	int ret;
> > +
> > +	/* Fixed parameters format doesn't require validation. */
> > +	if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS)
> > +		return 0;

You need to add a check to rkisp1_params_s_fmt_meta_out() to reject
format changes once the queue is busy, or you'll have bad surprises.

> > +
> > +	ret = rkisp1_params_validate_ext_params(params, cfg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * If the parameters buffer is valid, copy it to the internal scratch
> > +	 * buffer to avoid userspace modifying the buffer content while
> > +	 * the driver processes it.
> > +	 */

You need to swap the validation and memcpy(), otherwise userspace could
modify it after you have validated the contents and before the copy.

> > +	memcpy(params_buf->cfg, cfg, sizeof(*cfg));
>
> I think that this part is something that we probably ought to handle
> in vb2 core if we can, since the problem it's fixing isn't specific to
> the extensible parameters format or even the rkisp1 itself (unless I'm
> missing something). That doesn't have to block this set though, we can
> change this over to a vb2-core implementation when that's done.

I like the idea. I think we can experiment with it in rkisp1, and then
move it to vb2 when adding a second implementation (likely C55 ?).

> > +
> > +	return 0;
> > +}
> > +
> >   static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
> >   {
> >   	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > @@ -2455,6 +2546,9 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
> >   
> >   static const struct vb2_ops rkisp1_params_vb2_ops = {
> >   	.queue_setup = rkisp1_params_vb2_queue_setup,
> > +	.buf_init = rkisp1_params_vb2_buf_init,
> > +	.buf_cleanup = rkisp1_params_vb2_buf_cleanup,
> > +	.buf_out_validate = rkisp1_params_vb2_buf_out_validate,
> >   	.wait_prepare = vb2_ops_wait_prepare,
> >   	.wait_finish = vb2_ops_wait_finish,
> >   	.buf_queue = rkisp1_params_vb2_buf_queue,

-- 
Regards,

Laurent Pinchart