media: v4l2-isp: reject zero-sized parameter blocks
PATCH v4 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. A block whose type_info[] entry is an uninitialised hole passes every check on the way there: a size of 0 is not caught by the block->size > buffer_size test, and the match against the type info size compares 0 with the hole's own 0 and passes as well. 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 whose type info entry is empty. The driver does not implement the type, so it 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. The size match then always runs against a non-zero size, and no block can advance the walk by zero.
---
v4:
- drop the block->size < sizeof(*block) check, redundant now that an empty type info entry is rejected before the size match (Jacopo)
- commit message reworked around the type info check
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 | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-isp.c +14 -1
@@ -99,12 +99,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
Hi David
Quoted 22 lines · David Carlier
v4l2_isp_params_validate_buffer()walks the blocks of a parameters buffer by addingblock->sizeto the current offset. A block whose type_info[] entry is an uninitialised hole passes every check on the way there: a size of 0 is not caught by theblock->size>buffer_sizetest, and the match against the type info size compares 0 with the hole's own 0 and passes as well. 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 whose type info entry is empty. The driver does not implement the type, so it 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. The size match then always runs against a non-zero size, and no block can advance the walk by zero.
Quoted 4 lines
Fixes 3cb6de6fafb8 ("media: v4l2-core: Introduce v4l2-isp.c")Suggested-by Jacopo Mondi <[email protected]>Signed-off-by David Carlier <[email protected]>
Thank you for the new version
Quoted 53 lines
---
v4:
- drop theblock->size<sizeof(*block)check, redundant now that an empty type info entry is rejected before the size match (Jacopo)
- commit message reworked around the type info checkv3:
- 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 | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)drivers/media/v4l2-core/v4l2-isp.c +14
-2@@ -99,12 +99,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