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, so skip such a block instead of failing the whole buffer: drivers may reserve uAPI block types they do not implement yet, and are free to ignore the block when processing the buffer.

---
v2:
- skip a block whose type info entry is empty instead of matching it against a zeroed entry, so a type the driver does not implement is ignored rather than failing the whole buffer (Jacopo)
- reworded the commit message, which no longer leans on
rppx1: its missing AWBG_POST entry is being fixed at
https://patchwork.linuxtv.org/project/linux-media/list/?series=29170

 drivers/media/v4l2-core/v4l2-isp.c | 41 ++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 13 deletions(-)

index 1eb46e080afa..439477b2b941 100644
@@ -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;

@@ -100,23 +107,31 @@ int v4l2_isp_params_validate_buffer(struct device *dev, struct vb2_buffer *vb,
 		}
 
 		/*
-		 * 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.
+		 * An empty type info entry denotes a block type the driver
+		 * does not support. Skip the block, it is up to the driver to
+		 * ignore it when processing the buffer.
 		 */
 		info = &type_info[block->type];
-		if (block->size != info->size &&
-		    (!(block->flags & V4L2_ISP_PARAMS_FL_BLOCK_DISABLE) ||
-		    block->size != sizeof(*block))) {
-			dev_dbg(dev,
-				"Invalid block size %u (expected %zu) at offset %zu\n",
-				block->size, info->size, block_offset);
-			return -EINVAL;
+		if (info->size) {
+			/*
+			 * 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.
+			 */
+			if (block->size != info->size &&
+			    (!(block->flags & V4L2_ISP_PARAMS_FL_BLOCK_DISABLE) ||
+			    block->size != sizeof(*block))) {
+				dev_dbg(dev,
+					"Invalid block size %u (expected %zu) at offset %zu\n",
+					block->size, info->size, block_offset);
+				return -EINVAL;
+			}
+
+			if (info->block_validate &&
+			    info->block_validate(dev, block))
+				return -EINVAL;
 		}
 
-		if (info->block_validate && info->block_validate(dev, block))
-			return -EINVAL;
-
 		block_offset += block->size;
 		buffer_size -= block->size;
 	}

2.55.0

Hi David

Hi Jacopo,

Yes, let's do that. It costs nothing either: with the header size check the walk always makes progress, so skipping is no longer needed to avoid the stall.

v3 keeps that check and replaces the skip with:

      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;
      }

which also leaves the rest of the loop unindented.

Cheers.

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.

---
 drivers/media/v4l2-core/v4l2-isp.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

index 1eb46e080afa..efe994b4c4d7 100644
@@ -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))) {

2.55.0

Oupsie, sent with the wrong subject prefix and without its changelog. It has:

v3:
- reject a block whose type info entry is empty instead of skipping it, so an unimplemented type does not become unconstrained uAPI

Let me know if you want it resent properly labelled.

Cheers.

Hi David

The subject seems right to me ?

More than this, you sent v3 in reply to v2 which I think possibly confuses tooling (not patchwork though, which correctly identifies this one as a separate series [1])

Would you like to resend as a separate thread ? Sorry for bothering

[1] https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/