media: v4l2-isp: reject zero-sized parameter blocks
PATCH v3 1/1 media: v4l2-isp: reject zero-sized parameter blocks
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.
---
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(-)
drivers/media/v4l2-core/v4l2-isp.c +21 -1
@@ -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 ;
@@ -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))) {
Signature · 1 line
2.55.0
Jacopo Mondi
maintainerHi David
Quoted 51 lines · David Carlier
v4l2_isp_params_validate_buffer()walks the blocks of a parameters buffer by addingblock->sizeto the current offset, but never bounds that size from below. A block with size 0 is not caught by theblock->size>buffer_sizetest, and the comparison againstinfo->sizepasses 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_QBUFon 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")Suggested-by Jacopo Mondi <[email protected]>Signed-off-by David Carlier <[email protected]>---
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 rppx1drivers/media/v4l2-core/v4l2-isp.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)drivers/media/v4l2-core/v4l2-isp.c +0
-0
drivers/media/v4l2-core/v4l2-isp.c int v4l2_isp_params_validate_buffer(struct device *dev, struct vb2_buffer *vb, · line 93
Quoted 10 lines
drivers/media/v4l2-core/v4l2-isp.c +7
-0@@ -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.
Quoted 33 lines
drivers/media/v4l2-core/v4l2-isp.c +14
-2if (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