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

[Dan Scally <dan.scally@ideasonboard.com>]

Hi Jacopo. As mentioned in the last patch, I think that this could be squashed into 5/8, but a 
couple of comments below

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;
>   	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);
> +	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;
> +	}
> +
> +	params_buf->cfg = kvmalloc(buf_size, GFP_KERNEL);
> +	if (IS_ERR_OR_NULL(params_buf->cfg))
> +		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);
> +}
> +
> +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;
> +
> +		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;
> +
> +	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.
> +	 */
> +	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.
> +
> +	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,