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

  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(-)
  

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,
  

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,
  

Hi
On Wed, Jun 12, 2024 at 07:20:48PM GMT, Laurent Pinchart wrote:
> 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.

I'm not sure I agree here

Squashing would create a patch that does too many things. Moving it
before the introduction of the extensible format would not make much
sense, as there would be nothing to validate/copy for the fixed
format.

I'll keep the ordering I have here if not a big deal.
>
> > 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 ?
>

Copying the buffer in ? Not sure I agree. The extensible format allows
userspace to change the memory layout of the buffer, if this happens
while the driver parses it, we'll ooops. With the fixed format the
memory layout is fixed, if userspace changes the content of the buffer
(ie the configuration parameters) after qbuf, well, it's their
problem. Copying (and validation) is costly and if it serves to avoid
an ooops it's more than justified. In the other case I'm not sure.

> > > +
> > > +	params_buf->cfg = kvmalloc(buf_size, GFP_KERNEL);
> > > +	if (IS_ERR_OR_NULL(params_buf->cfg))
>
> Can kvmalloc() return an error pointer ?
>

I think I got a report somewhere about this but I can't find it
anymore :/

> > > +		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 ?).
>

Again, not sure. Copying and buffer allocations are costly. If we risk
an invalid memory access like in the extensible format case then I
agree it's worth it. In all other cases, if userspace changes a buffer
content after qbuf we can't do much about it (is it any different than
userspace modifying the content of an output video buffer after it has
been queued ?)

> > > +
> > > +	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
  

Hi Jacopo,

On Wed, Jun 19, 2024 at 04:22:00PM +0200, Jacopo Mondi wrote:
> On Wed, Jun 12, 2024 at 07:20:48PM GMT, Laurent Pinchart wrote:
> > 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.
> 
> I'm not sure I agree here
> 
> Squashing would create a patch that does too many things. Moving it
> before the introduction of the extensible format would not make much
> sense, as there would be nothing to validate/copy for the fixed
> format.

I think the copy should be done for the fixed format too. See below.

> I'll keep the ordering I have here if not a big deal.
>
> > > 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 ?
> 
> Copying the buffer in ? Not sure I agree. The extensible format allows
> userspace to change the memory layout of the buffer, if this happens
> while the driver parses it, we'll ooops. With the fixed format the
> memory layout is fixed, if userspace changes the content of the buffer
> (ie the configuration parameters) after qbuf, well, it's their
> problem.

Can you guarantee that the driver will never ever crash or corrupt
memory if that happens ? Especially if you take into account the fact
that the compiler and CPU can reorder accesses, as well as split and
merge accesses (i.e. reading the same memory location multiple times
when it's accessed once only in the code), that seems hard to do.

> Copying (and validation) is costly and if it serves to avoid
> an ooops it's more than justified. In the other case I'm not sure.

How expensive is the copy ?

> > > > +
> > > > +	params_buf->cfg = kvmalloc(buf_size, GFP_KERNEL);
> > > > +	if (IS_ERR_OR_NULL(params_buf->cfg))
> >
> > Can kvmalloc() return an error pointer ?
> 
> I think I got a report somewhere about this but I can't find it
> anymore :/
> 
> > > > +		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 ?).
> 
> Again, not sure. Copying and buffer allocations are costly. If we risk

I'm not saying we should do the copy unconditionally in vb2, but vb2
should provide the feature for drivers that want to use it.

> an invalid memory access like in the extensible format case then I
> agree it's worth it. In all other cases, if userspace changes a buffer
> content after qbuf we can't do much about it (is it any different than
> userspace modifying the content of an output video buffer after it has
> been queued ?)

It is. If you change the contents of a video buffer, you'll have corrupt
frames in the worst case. If you change the contents of memory used by
the CPU to influence kernel code flows, it's an entirely different
story.

> > > > +
> > > > +	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,
  

Hi Laurent

On Wed, Jun 19, 2024 at 06:44:20PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Jun 19, 2024 at 04:22:00PM +0200, Jacopo Mondi wrote:
> > On Wed, Jun 12, 2024 at 07:20:48PM GMT, Laurent Pinchart wrote:
> > > 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.
> >
> > I'm not sure I agree here
> >
> > Squashing would create a patch that does too many things. Moving it
> > before the introduction of the extensible format would not make much
> > sense, as there would be nothing to validate/copy for the fixed
> > format.
>
> I think the copy should be done for the fixed format too. See below.
>
> > I'll keep the ordering I have here if not a big deal.
> >
> > > > 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 ?
> >
> > Copying the buffer in ? Not sure I agree. The extensible format allows
> > userspace to change the memory layout of the buffer, if this happens
> > while the driver parses it, we'll ooops. With the fixed format the
> > memory layout is fixed, if userspace changes the content of the buffer
> > (ie the configuration parameters) after qbuf, well, it's their
> > problem.
>
> Can you guarantee that the driver will never ever crash or corrupt
> memory if that happens ? Especially if you take into account the fact

All of the mainline driver work in this way today. But ok if most
people think it's worth it, I'll do so and re-order the patches.

> that the compiler and CPU can reorder accesses, as well as split and
> merge accesses (i.e. reading the same memory location multiple times
> when it's accessed once only in the code), that seems hard to do.
>
> > Copying (and validation) is costly and if it serves to avoid
> > an ooops it's more than justified. In the other case I'm not sure.
>
> How expensive is the copy ?
>

For sure it doubles the allocated memory as we've one backing memory
buffer for each buffer requested by userspace.
  

On Wed, Jun 19, 2024 at 05:55:12PM +0200, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Wed, Jun 19, 2024 at 06:44:20PM GMT, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Wed, Jun 19, 2024 at 04:22:00PM +0200, Jacopo Mondi wrote:
> > > On Wed, Jun 12, 2024 at 07:20:48PM GMT, Laurent Pinchart wrote:
> > > > 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.
> > >
> > > I'm not sure I agree here
> > >
> > > Squashing would create a patch that does too many things. Moving it
> > > before the introduction of the extensible format would not make much
> > > sense, as there would be nothing to validate/copy for the fixed
> > > format.
> >
> > I think the copy should be done for the fixed format too. See below.
> >
> > > I'll keep the ordering I have here if not a big deal.
> > >
> > > > > 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 ?
> > >
> > > Copying the buffer in ? Not sure I agree. The extensible format allows
> > > userspace to change the memory layout of the buffer, if this happens
> > > while the driver parses it, we'll ooops. With the fixed format the
> > > memory layout is fixed, if userspace changes the content of the buffer
> > > (ie the configuration parameters) after qbuf, well, it's their
> > > problem.
> >
> > Can you guarantee that the driver will never ever crash or corrupt
> > memory if that happens ? Especially if you take into account the fact
> 
> All of the mainline driver work in this way today. But ok if most
> people think it's worth it, I'll do so and re-order the patches.

Unless the memcpy really hinders performances, I think we should ensure
safety first and foremest.

> > that the compiler and CPU can reorder accesses, as well as split and
> > merge accesses (i.e. reading the same memory location multiple times
> > when it's accessed once only in the code), that seems hard to do.
> >
> > > Copying (and validation) is costly and if it serves to avoid
> > > an ooops it's more than justified. In the other case I'm not sure.
> >
> > How expensive is the copy ?
> 
> For sure it doubles the allocated memory as we've one backing memory
> buffer for each buffer requested by userspace.

True, but parameter buffers are quite small. I haven't checked but I
would expect everything to hold in one page.
  

Hi Laurent

On Wed, Jun 19, 2024 at 07:11:01PM GMT, Laurent Pinchart wrote:
> On Wed, Jun 19, 2024 at 05:55:12PM +0200, Jacopo Mondi wrote:
> > Hi Laurent
> >
> > On Wed, Jun 19, 2024 at 06:44:20PM GMT, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > On Wed, Jun 19, 2024 at 04:22:00PM +0200, Jacopo Mondi wrote:
> > > > On Wed, Jun 12, 2024 at 07:20:48PM GMT, Laurent Pinchart wrote:
> > > > > 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.
> > > >
> > > > I'm not sure I agree here
> > > >
> > > > Squashing would create a patch that does too many things. Moving it
> > > > before the introduction of the extensible format would not make much
> > > > sense, as there would be nothing to validate/copy for the fixed
> > > > format.
> > >
> > > I think the copy should be done for the fixed format too. See below.
> > >
> > > > I'll keep the ordering I have here if not a big deal.
> > > >
> > > > > > 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 ?
> > > >
> > > > Copying the buffer in ? Not sure I agree. The extensible format allows
> > > > userspace to change the memory layout of the buffer, if this happens
> > > > while the driver parses it, we'll ooops. With the fixed format the
> > > > memory layout is fixed, if userspace changes the content of the buffer
> > > > (ie the configuration parameters) after qbuf, well, it's their
> > > > problem.
> > >
> > > Can you guarantee that the driver will never ever crash or corrupt
> > > memory if that happens ? Especially if you take into account the fact
> >
> > All of the mainline driver work in this way today. But ok if most
> > people think it's worth it, I'll do so and re-order the patches.
>
> Unless the memcpy really hinders performances, I think we should ensure
> safety first and foremest.
>

I haven't run any test, but I doubt the performance penality is
terrible

> > > that the compiler and CPU can reorder accesses, as well as split and
> > > merge accesses (i.e. reading the same memory location multiple times
> > > when it's accessed once only in the code), that seems hard to do.
> > >
> > > > Copying (and validation) is costly and if it serves to avoid
> > > > an ooops it's more than justified. In the other case I'm not sure.
> > >
> > > How expensive is the copy ?
> >
> > For sure it doubles the allocated memory as we've one backing memory
> > buffer for each buffer requested by userspace.
>
> True, but parameter buffers are quite small. I haven't checked but I
> would expect everything to hold in one page.

True that.

Ok, safety first, I'll add support for copying in the fixed format as
well!

Thanks
  j

>
> --
> Regards,
>
> Laurent Pinchart