Re: [PATCH v3] media: v4l2-isp: reject zero-sized parameter blocks

[Jacopo Mondi]

Hi David

On Thu, Aug 20, 2026 at 12:17:22PM +0100, David Carlier wrote:
> v4l2_isp_params_validate_buffer() walks the blocks of a parameters
> buffer by adding block->size to the current offset, but never bounds
> that size from below. A block with size 0 is not caught by the
> block->size > buffer_size test, and the comparison against info->size
> passes as well when the driver's type_info[] entry is an uninitialised
> hole, both sizes being 0. The walk then makes no forward progress and
> loops forever.
>
> Drivers build their type_info[] arrays with designated initialisers
> indexed by their block type enumeration, so an enumerator left without
> an entry leaves a zeroed hole rather than failing the build. Drivers
> call the validator from vb2 .buf_prepare, so such a hole turns a
> VIDIOC_QBUF on the parameters video device into an unkillable task
> spinning with the queue mutex held.
>
> Reject a block smaller than its own header. A block's size includes its
> header, so anything below that is malformed whatever the driver table
> contains, and rejecting it is what keeps the walk moving. Blocks
> carrying only a header to disable a block are exactly that size and
> still pass.
>
> An empty type info entry can then no longer stall the walk, but a block
> matched against it is only constrained by that header size check. Reject
> such a block explicitly: the driver does not implement the type and
> cannot tell whether the block content is meaningful, and accepting it
> silently would leave that content unconstrained until a later kernel
> implements the type and starts validating it.
>
> Fixes: 3cb6de6fafb8 ("media: v4l2-core: Introduce v4l2-isp.c")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Signed-off-by: David Carlier <devnexen@xxxxxxxxx>
> ---
> v3:
> - reject a block whose type info entry is empty instead of skipping
>   it, so a type the driver does not implement cannot become
>   unconstrained uAPI (Jacopo)
>
> v2:
> - skip an empty type info entry instead of matching the block against
>   a zeroed one
> - reworded the commit message, which no longer leans on rppx1
>
>  drivers/media/v4l2-core/v4l2-isp.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-isp.c b/drivers/media/v4l2-core/v4l2-isp.c
> index 1eb46e080afa..efe994b4c4d7 100644
> --- a/drivers/media/v4l2-core/v4l2-isp.c
> +++ b/drivers/media/v4l2-core/v4l2-isp.c
> @@ -84,6 +84,13 @@ int v4l2_isp_params_validate_buffer(struct device *dev, struct vb2_buffer *vb,
>  			return -EINVAL;
>  		}
>
> +		if (block->size < sizeof(*block)) {
> +			dev_dbg(dev,
> +				"Invalid block size %u at offset %zu\n",
> +				block->size, block_offset);
> +			return -EINVAL;
> +		}
> +

I'm sorry, but now that we refuse empty block info with size == 0,
wouldn't this be caught by the below

		if (block->size != info->size &&
		    (!(block->flags & V4L2_ISP_PARAMS_FL_BLOCK_DISABLE) ||
		    block->size != sizeof(*block))) {

Do we need to check it here as well ?

nit: the dev_dbg() line fits on 2 lines only.


>  		if (block->size > buffer_size) {
>  			dev_dbg(dev, "Premature end of parameters data\n");
>  			return -EINVAL;
> @@ -99,12 +106,25 @@ int v4l2_isp_params_validate_buffer(struct device *dev, struct vb2_buffer *vb,
>  			return -EINVAL;
>  		}
>
> +		/*
> +		 * An empty type info entry denotes a block type the driver
> +		 * does not support. Reject the buffer instead of ignoring the
> +		 * block: accepting it silently would let userspace fill it
> +		 * with data that a later kernel, once it implements the type,
> +		 * would validate and possibly reject.
> +		 */
> +		info = &type_info[block->type];
> +		if (!info->size) {
> +			dev_dbg(dev, "Unsupported block type %u at offset %zu\n",
> +				block->type, block_offset);
> +			return -EINVAL;
> +		}
> +
>  		/*
>  		 * Match the block reported size against the type info provided
>  		 * one, but allow the block to only contain the header in
>  		 * case it is going to be disabled.
>  		 */
> -		info = &type_info[block->type];
>  		if (block->size != info->size &&
>  		    (!(block->flags & V4L2_ISP_PARAMS_FL_BLOCK_DISABLE) ||
>  		    block->size != sizeof(*block))) {
> --
> 2.55.0
>