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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi David

On Sat, Aug 15, 2026 at 08:38:39PM +0100, David Carlier wrote:
> v4l2_isp_params_validate_buffer() advances by block->size but never
> bounds it from below. A block with size 0 passes both the
> block->size > buffer_size test and the comparison against info->size
> whenever the driver's type_info[] entry is an uninitialised hole, so

This shouldn't happen (an empty type_info[] I mean)

> the walk makes no forward progress and loops forever.
>
> Drivers call this from vb2 .buf_prepare, so VIDIOC_QBUF on the
> parameters video device hangs the calling task with the queue mutex
> held. rppx1 has such a hole today; fix the core so the walk terminates

Uh, where ?

I see

static const struct v4l2_isp_params_block_type_info
rppx1_ext_params_blocks_info[] = {
	RPPX1_PARAMS_BLOCK_INFO(BLS_PRE1, bls),
	RPPX1_PARAMS_BLOCK_INFO(BLS_PRE2, bls),
	RPPX1_PARAMS_BLOCK_INFO(LIN_PRE1, lin),
	RPPX1_PARAMS_BLOCK_INFO(LIN_PRE2, lin),
	RPPX1_PARAMS_BLOCK_INFO(LSC_PRE1, lsc),
	RPPX1_PARAMS_BLOCK_INFO(LSC_PRE2, lsc),
	RPPX1_PARAMS_BLOCK_INFO(AWBG_PRE1, awbg),
	RPPX1_PARAMS_BLOCK_INFO(AWBG_PRE2, awbg),
	RPPX1_PARAMS_BLOCK_INFO(CCOR_POST, ccor),
	RPPX1_PARAMS_BLOCK_INFO(HIST_PRE1, hist),
	RPPX1_PARAMS_BLOCK_INFO(HIST_PRE2, hist),
	RPPX1_PARAMS_BLOCK_INFO(HIST_POST, hist),
	RPPX1_PARAMS_BLOCK_INFO(EXM_PRE1, exm),
	RPPX1_PARAMS_BLOCK_INFO(EXM_PRE2, exm),
	RPPX1_PARAMS_BLOCK_INFO(WBMEAS_POST, wbmeas),
	RPPX1_PARAMS_BLOCK_INFO(GA_HV, ga),
	RPPX1_PARAMS_BLOCK_INFO(GA_MV, ga),
};

being passed to v4l2_isp_params_validate_buffer().

> for any driver.
>
> Require every block to be at least as large as its header. Blocks
> carrying only a header to disable a block are exactly that size and
> still pass.
>
> Fixes: 3cb6de6fafb8 ("media: v4l2-core: Introduce v4l2-isp.c")
> Cc: stable@vger.kernel.org
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-isp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-isp.c b/drivers/media/v4l2-core/v4l2-isp.c
> index 1eb46e080afa..760e16a5ba9d 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 not against this, but it only makes sense if the driver populates
the list of v4l2_isp_params_block_type_info[] with an empty item,
which shouldn't happen.

>  		if (block->size > buffer_size) {
>  			dev_dbg(dev, "Premature end of parameters data\n");
>  			return -EINVAL;
> --
> 2.55.0
>