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

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

[David Carlier]

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
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
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;
+		}
+
 		if (block->size > buffer_size) {
 			dev_dbg(dev, "Premature end of parameters data\n");
 			return -EINVAL;
-- 
2.55.0

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

[Jacopo Mondi]

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
>

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

[David CARLIER]

Hi Jacopo,

On Mon, Aug 17, 2026 at 11:59:15AM +0200, Jacopo Mondi wrote:
> > whenever the driver's type_info[] entry is an uninitialised hole, so
>
> This shouldn't happen (an empty type_info[] I mean)
[...]
> > held. rppx1 has such a hole today; fix the core so the walk terminates
>
> Uh, where ?

Sorry, I should have spelled this out in the commit message.

RPPX1_PARAMS_BLOCK_INFO() is a designated initialiser indexed by the
block type (rpp_params.c:13):

      #define RPPX1_PARAMS_BLOCK_INFO(block, data) \
              [RPPX1_PARAMS_BLOCK_TYPE_ ## block] = { \
                      .size = sizeof(struct rppx1_ ## data ## _params), \
              }

so the list reads as dense but is indexed by the enum. It has 17
entries for 18 enumerators, and the missing one is AWBG_POST (== 3).
The last index used is LIN_PRE2 (== 17), so ARRAY_SIZE() is still 18
and the entry at 3 is simply zeroed.

A block with type AWBG_POST and size 0 then gets through: 0 is not
larger than the remaining buffer, and block->size != info->size is
0 != 0. block_offset and buffer_size don't move and the loop spins,
in .buf_prepare, holding the queue mutex.

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

True, and rppx1 is the only one that does - rkisp1, c3-isp and
mali-c55 all look complete. What bothers me is that the loop only
terminates if every driver's table is right, and nothing checks that
at build time. A missing line in a driver would be a rejected buffer
rather than a stuck task. And a block size below its own header is
malformed anyway, whatever the type.

That said, rppx1 needs a patch either way: AWBG_POST is documented i
rppx1-config.h and the driver probes rpp->post.awbg, but the block
can't be used today - the size check compares against 0, and
rppx1_params() has no case for it in the switch.

Happy to drop a separate patch and keep only the driver fix if you prefer.

Cheers.

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

[Jacopo Mondi]

Hi David

On Mon, Aug 17, 2026 at 02:40:53PM +0100, David CARLIER wrote:
> Hi Jacopo,
>
> On Mon, Aug 17, 2026 at 11:59:15AM +0200, Jacopo Mondi wrote:
> > > whenever the driver's type_info[] entry is an uninitialised hole, so
> >
> > This shouldn't happen (an empty type_info[] I mean)
> [...]
> > > held. rppx1 has such a hole today; fix the core so the walk terminates
> >
> > Uh, where ?
>
> Sorry, I should have spelled this out in the commit message.
>
> RPPX1_PARAMS_BLOCK_INFO() is a designated initialiser indexed by the
> block type (rpp_params.c:13):
>
>       #define RPPX1_PARAMS_BLOCK_INFO(block, data) \
>               [RPPX1_PARAMS_BLOCK_TYPE_ ## block] = { \
>                       .size = sizeof(struct rppx1_ ## data ## _params), \
>               }
>
> so the list reads as dense but is indexed by the enum. It has 17

Upsie, you're right, I didn't properly consider that..


> entries for 18 enumerators, and the missing one is AWBG_POST (== 3).
> The last index used is LIN_PRE2 (== 17), so ARRAY_SIZE() is still 18
> and the entry at 3 is simply zeroed.
>
> A block with type AWBG_POST and size 0 then gets through: 0 is not
> larger than the remaining buffer, and block->size != info->size is
> 0 != 0. block_offset and buffer_size don't move and the loop spins,
> in .buf_prepare, holding the queue mutex.
>
> > 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.
>
> True, and rppx1 is the only one that does - rkisp1, c3-isp and
> mali-c55 all look complete. What bothers me is that the loop only
> terminates if every driver's table is right, and nothing checks that
> at build time. A missing line in a driver would be a rejected buffer

Right.

Should we instead deman that all entries provided by the driver are
populated by failing validation if info->size == 0 ?

> rather than a stuck task. And a block size below its own header is
> malformed anyway, whatever the type.
>
> That said, rppx1 needs a patch either way: AWBG_POST is documented i
> rppx1-config.h and the driver probes rpp->post.awbg, but the block
> can't be used today - the size check compares against 0, and
> rppx1_params() has no case for it in the switch.

Right.. you might have seen ?
https://patchwork.linuxtv.org/project/linux-media/list/?series=29170

In any case, yes, I think we should populate all entries and either
require all info blocks to be valid (hence a 0 sized block from
userspace is refused) or explicitly check if block->size > 0
as you're doing here.


>
> Happy to drop a separate patch and keep only the driver fix if you prefer.
>
> Cheers.

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

[David CARLIER]

On Mon, Aug 17, 2026 at 05:56:30PM +0200, Jacopo Mondi wrote:
> Should we instead deman that all entries provided by the driver are
> populated by failing validation if info->size == 0 ?

Good idea, and I'd rather keep both in v2 if you don't mind - they
guard different things.

info->size == 0 is about the driver table, and it lets us answer
"this driver doesn't handle that type" rather than carrying on with
an empty entry and a NULL block_validate.

block->size < sizeof(*block) is about the buffer coming from
userspace: a block's size includes its header, so anything smaller is
malformed no matter what the table says, and it keeps the loop moving
without depending on the table at all.

Both are a couple of lines, so dropping either one is no trouble if
you prefer.

> Right.. you might have seen ?
> https://patchwork.linuxtv.org/project/linux-media/list/?series=29170

I hadn't, thanks. That takes care of the rppx1 side, so there's
nothing left for me to send there and I'll stick to the core change.

I'll reword the commit message so it doesn't lean on rppx1, and keep
Cc: stable unless you disagree.

Cheers.