Message-ID: <ylea2fwhdzpo6fqkgk4g5frlriiawd5lyn6vjbghcmswfmy7hq@j7jksyyjkzkb>
Date: Mon, 6 Oct 2025 12:28:01 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Dafna Hirschfeld <dafna@...tmail.com>, Keke Li <keke.li@...ogic.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Heiko Stuebner <heiko@...ech.de>, 
	Dan Scally <dan.scally@...asonboard.com>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Antoine Bouyer <antoine.bouyer@....com>, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	linux-rockchip@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 5/8] media: v4l2-core: Introduce v4l2-isp.c

Hi Laurent

On Mon, Oct 06, 2025 at 04:08:06AM +0300, Laurent Pinchart wrote:
> On Mon, Oct 06, 2025 at 03:47:43AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Mon, Sep 15, 2025 at 07:18:14PM +0200, Jacopo Mondi wrote:
> > > Add to the v4l2 framework helper functions to support drivers
> >
> > s/v4l2/V4L2/
> >
> > > when validating a buffer of extensible ISP parameters.
> > >
> > > Introduce new types in include/media/v4l2-isp.h that drivers shall use
> > > in order to comply with the generic ISP parameters validation procedure,
> > > and add helper functionss to v4l2-isp.c to perform blocks and buffer
> > > validation.
> > >
> > > Reviewed-by: Daniel Scally <dan.scally@...asonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > > ---
> > >  MAINTAINERS                        |   2 +
> > >  drivers/media/v4l2-core/Kconfig    |   4 ++
> > >  drivers/media/v4l2-core/Makefile   |   1 +
> > >  drivers/media/v4l2-core/v4l2-isp.c | 108 +++++++++++++++++++++++++++++++++++++
> > >  include/media/v4l2-isp.h           | 100 ++++++++++++++++++++++++++++++++++
> > >  5 files changed, 215 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index abba872cb63f1430a49a2afbace4b9f9958c3991..5e0e4208ebe6c58a9ea0834e1ebb36abd2de06e1 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -26415,6 +26415,8 @@ M:	Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > >  L:	linux-media@...r.kernel.org
> > >  S:	Maintained
> > >  F:	Documentation/userspace-api/media/v4l/extensible-parameters.rst
> > > +F:	drivers/media/v4l2-core/v4l2-isp.c
> > > +F:	include/media/v4l2-isp.h
> > >  F:	include/uapi/linux/media/v4l2-isp.h
> > >
> > >  VF610 NAND DRIVER
> > > diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> > > index 331b8e535e5bbf33f22638b2ae8bc764ad5fc407..d50ccac9733cc39a43426ae7e7996dd0b5b45186 100644
> > > --- a/drivers/media/v4l2-core/Kconfig
> > > +++ b/drivers/media/v4l2-core/Kconfig
> > > @@ -82,3 +82,7 @@ config V4L2_CCI_I2C
> > >  	depends on I2C
> > >  	select REGMAP_I2C
> > >  	select V4L2_CCI
> > > +
> > > +config V4L2_ISP
> > > +	tristate
> > > +	depends on VIDEOBUF2_CORE
> > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > > index 2177b9d63a8ffc1127c5a70118249a2ff63cd759..329f0eadce994cc1c8580beb435f68fa7e2a7aeb 100644
> > > --- a/drivers/media/v4l2-core/Makefile
> > > +++ b/drivers/media/v4l2-core/Makefile
> > > @@ -29,6 +29,7 @@ obj-$(CONFIG_V4L2_CCI) += v4l2-cci.o
> > >  obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash-led-class.o
> > >  obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> > >  obj-$(CONFIG_V4L2_H264) += v4l2-h264.o
> > > +obj-$(CONFIG_V4L2_ISP) += v4l2-isp.o
> > >  obj-$(CONFIG_V4L2_JPEG_HELPER) += v4l2-jpeg.o
> > >  obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
> > >  obj-$(CONFIG_V4L2_VP9) += v4l2-vp9.o
> > > diff --git a/drivers/media/v4l2-core/v4l2-isp.c b/drivers/media/v4l2-core/v4l2-isp.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..e350bdaf53b5502e1ec2a4989c20df1100ab2d2a
> > > --- /dev/null
> > > +++ b/drivers/media/v4l2-core/v4l2-isp.c
> > > @@ -0,0 +1,108 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Video4Linux2 generic ISP parameters and statistics support
> > > + *
> > > + * Copyright (C) 2025 Ideas On Board Oy
> > > + * Author: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > > + */
> > > +
> > > +#include <linux/bitops.h>
> > > +#include <linux/device.h>
> > > +
> > > +#include <media/videobuf2-core.h>
> > > +#include <media/v4l2-isp.h>
> >
> > v4l2-isp goes first.
> >
> > > +
> > > +int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
> > > +				size_t max_size)
> > > +{
> > > +	size_t header_size = offsetof(struct v4l2_params_buffer, data);
> > > +	struct v4l2_params_buffer *buffer = vb2_plane_vaddr(vb, 0);
> > > +	size_t payload_size = vb2_get_plane_payload(vb, 0);
> > > +	size_t buffer_size;
> > > +
> > > +	/* Payload size can't be greater than the destination buffer size */
> > > +	if (payload_size > max_size) {
> > > +		dev_dbg(dev, "Payload size is too large: %zu\n", payload_size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Payload size can't be smaller than the header size */
> > > +	if (payload_size < header_size) {
> > > +		dev_dbg(dev, "Payload size is too small: %zu\n", payload_size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Validate the size reported in the parameter buffer header */
> > > +	buffer_size = header_size + buffer->data_size;
> > > +	if (buffer_size != payload_size) {
> > > +		dev_dbg(dev, "Data size %zu and payload size %zu are different\n",
> > > +			buffer_size, payload_size);
> > > +		return -EINVAL;
> > > +	}
> >
> > This check needs to go to v4l2_params_blocks_validate() as it has to be
> > performed on the data after copying.
> >

I'm not sure. The aim of pre-validation (if we want to call it in this
way) is to check the correctness of the userspace provided buffer, and
the function as its main argument the vb2 buffer pointer for this
reason.

Isn't it important to check that the sizes are correct before doing a
mem copy ?

> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_params_buffer_validate);
> > > +
> > > +int v4l2_params_blocks_validate(struct device *dev,
> > > +				const struct v4l2_params_buffer *buffer,
> > > +				const struct v4l2_params_handler *handlers,
> > > +				size_t num_handlers)

While the actual "validation" checks the content of v4l2_params_buffer
after it has been copied to a kernel-only memory location. If I would
have to check its size I would have to receive here the vb2 buffer size
as argument (which the drivers should have just used as argument to
the memcpy). It feels a bit mixing two things (that's also why I liked
having a 'buffer validate' and a 'blocks validate' function, but I
won't argue)

> > > +{
> > > +	size_t block_offset = 0;
> > > +	size_t buffer_size;
> > > +
> > > +	/* Walk the list of parameter blocks and validate them. */
> > > +	buffer_size = buffer->data_size;
> > > +	while (buffer_size >= sizeof(struct v4l2_params_block_header)) {
> > > +		const struct v4l2_params_handler *handler;
> > > +		const struct v4l2_params_block_header *block;
> > > +
> > > +		/* Validate block sizes and types against the handlers. */
> > > +		block = (const struct v4l2_params_block_header *)
> > > +			(buffer->data + block_offset);
> > > +
> > > +		if (block->type >= num_handlers) {
> > > +			dev_dbg(dev, "Invalid parameters block type\n");
> >
> > I'd print the type and offset in the message to ease debugging.
> >
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		if (block->size > buffer_size) {
> > > +			dev_dbg(dev, "Premature end of parameters data\n");
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		/* It's invalid to specify both ENABLE and DISABLE. */
> > > +		if ((block->flags & (V4L2_PARAMS_FL_BLOCK_ENABLE |
> > > +				     V4L2_PARAMS_FL_BLOCK_DISABLE)) ==
> > > +		     (V4L2_PARAMS_FL_BLOCK_ENABLE |
> > > +		     V4L2_PARAMS_FL_BLOCK_DISABLE)) {
> > > +			dev_dbg(dev, "Invalid parameters block flags\n");
> >
> > Same here (print the flags and offset).
> >
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		/*
> > > +		 * Match the block reported size against the handler's expected
> > > +		 * one, but allow the block to only contain the header in
> > > +		 * case it is going to be disabled.
> > > +		 */
> > > +		handler = &handlers[block->type];
> > > +		if (block->size != handler->size &&
> > > +		    (!(block->flags & V4L2_PARAMS_FL_BLOCK_DISABLE) ||
> > > +		    block->size != sizeof(*block))) {
> > > +			dev_dbg(dev, "Invalid parameters block size\n");
> >
> > And here too (print the size and offset).
> >
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		block_offset += block->size;
> > > +		buffer_size -= block->size;
> > > +	}
> > > +
> > > +	if (buffer_size) {
> > > +		dev_dbg(dev, "Unexpected data after the parameters buffer end\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_params_blocks_validate);
> > > diff --git a/include/media/v4l2-isp.h b/include/media/v4l2-isp.h
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..2ad62c6169eef3d0fb8d245de56cc6bd7e6227e4
> > > --- /dev/null
> > > +++ b/include/media/v4l2-isp.h
> > > @@ -0,0 +1,100 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Video4Linux2 generic ISP parameters and statistics support
> > > + *
> > > + * Copyright (C) 2025 Ideas On Board Oy
> > > + * Author: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > > + */
> > > +
> > > +#ifndef V4L2_PARAMS_H_
> > > +#define V4L2_PARAMS_H_
> >
> > V4L2_ISP_H_
> >
> > > +
> > > +#include <linux/media/v4l2-isp.h>
> > > +
> > > +struct device;
> > > +struct vb2_buffer;
> > > +
> > > +/**
> > > + * typedef v4l2_params_block_handler - V4L2 extensible format block handler
> >
> > As commented on 1/8, let's use the v4l2_isp_ prefix.
> >
> > > + * @arg: pointer the driver-specific argument
> > > + * @block: the ISP configuration block to handle
> > > + *
> > > + * Defines the function signature of the functions that handle an ISP block
> > > + * configuration.
> > > + */
> > > +typedef void (*v4l2_params_block_handler)(void *arg,
> > > +					  const struct v4l2_params_block_header *block);
> > > +
> > > +/**
> > > + * struct v4l2_params_handler - V4L2 extensible format handler
> > > + * @size: the block expected size
> > > + * @handler: the block handler function
> > > + *
> > > + * The v4l2_params_handler defines the type that driver making use of the
> > > + * V4L2 extensible parameters shall use to define their own ISP block
> > > + * handlers.
> > > + *
> > > + * Drivers shall prepare a list of handlers, one for each supported ISP block
> > > + * and correctly populate the structure's field with the expected block @size
> > > + * (used for validation) and a pointer to each block @handler function.
> > > + */
> > > +struct v4l2_params_handler {
> > > +	size_t size;
> > > +	v4l2_params_block_handler handler;
> > > +};
> > > +
> > > +/**
> > > + * v4l2_params_buffer_validate - Validate a V4L2 extensible parameters buffer
> >
> > As this is the pre-copy validation, what would you think of calling the
> > function v4l2_isp_params_pre_validate_buffer() ? The next function would
> > be called v4l2_isp_params_validate_buffer(), as they're both about
> > buffer validation. I'm also OK to keep the current names (with a
> > v4l2_isp_ prefix).
> >
> > I'm also thinking that the copy could be moved to the helper, but it can
> > be done later.
> >
> > > + * @dev: the driver's device pointer
> > > + * @vb: the videobuf2 buffer
> > > + * @max_size: the maximum allowed buffer size
> > > + * @buffer_validate: callback to the driver-specific buffer validation
> >
> > You forgot to drop the documentation for this argument.
> >
> > > + *
> > > + * Helper function that performs validation of an extensible parameters buffer.
> > > + *
> > > + * The helper is meant to be used by drivers to perform validation of the
> > > + * extensible parameters buffer size correctness.
> > > + *
> > > + * The @vb buffer as received from the vb2 .buf_prepare() operation is checked
> > > + * against @max_size and its validated to be large enough to accommodate at
> > > + * least one ISP configuration block. The effective buffer size is compared
> > > + * with the reported data size to make sure they match.
> > > + *
> > > + * Drivers should use this function to validate the buffer size correctness
> > > + * before performing a copy of the user-provided videobuf2 buffer content into a
> > > + * kernel-only memory buffer to prevent userspace from modifying the buffer
> > > + * content after it has been submitted to the driver.
> > > + */
> > > +int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
> > > +				size_t max_size);
> > > +
> > > +/**
> > > + * v4l2_params_blocks_validate - Validate V4L2 extensible parameters ISP
> > > + *				 configuration blocks
> > > + * @dev: the driver's device pointer
> > > + * @buffer: the extensible parameters configuration buffer
> > > + * @handlers: the list of block handlers
> >
> > array of block handlers
> >
> > > + * @num_handlers: the number of block handlers
> > > + *
> > > + * Helper function that performs validation of the ISP configuration blocks in
> > > + * an extensible parameters buffer.
> > > + *
> > > + * The helper is meant to be used by drivers to perform validation of the
> > > + * ISP configuration data blocks. For each block in the extensible parameters
> > > + * buffer, its size and correctness are validated against its associated handler
> > > + * in the @handlers list.
> >
> > You need to explain somewhere that the handlers array is indexed by
> > block type.
> >
> > > + *
> > > + * Drivers should use this function to validate the ISP configuration blocks
> > > + * after having validated the correctness of the vb2 buffer sizes by using the
> > > + * v4l2_params_buffer_validate() helper first. Once the buffer size has been
> > > + * validated, drivers should perform a copy of the user-provided buffer into a
> > > + * kernel-only memory buffer to prevent userspace from modifying the buffer
> > > + * content after it has been submitted to the driver, and then call this
> > > + * function to perform per-block validation.
> >
> > There's room for improvement in the documentation. I think it would be
> > clearer if you explained the big picture in
> > Documentation/userspace-api/media/v4l/extensible-parameters.rst
>
> My bad, that should be Documentation/driver-api/media/v4l2-isp.rst.
>
> > (pre-validation, copy and post-validation), and only focussed on what
> > those two functions do in their kerneldoc. That can be done later,
> > nothing that you say here is incorrect.
> >
> > With the other comments addressed,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
> >
> > > + */
> > > +int v4l2_params_blocks_validate(struct device *dev,
> > > +				const struct v4l2_params_buffer *buffer,
> > > +				const struct v4l2_params_handler *handlers,
> > > +				size_t num_handlers);
> > > +
> > > +#endif /* V4L2_PARAMS_H_ */
>
> --
> Regards,
>
> Laurent Pinchart
>

Powered by blists - more mailing lists