Re: [PATCH v4 2/7] media: qcom: camss: Add camss-params helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The result of `vb2_plane_vaddr()` is used without being checked for NULL, allowing unprivileged userspace to trigger a kernel crash via an unmapped or invalid buffer.
- [High] `camss_isp_params_apply` introduces a Double-Fetch (TOCTOU) vulnerability by re-reading size fields from a user-shared parameter buffer after validation, allowing malicious userspace to cause out-of-bounds reads or infinite kernel loops.
- [Low] The patch introduces a new file that relies on a non-existent UAPI header `<uapi/linux/camss-config.h>` and an undefined constant `CAMSS_PARAMS_MAX_PAYLOAD`, breaking the kernel build.

Pre-existing issues:
- [High] Preexisting vulnerability in core `v4l2_isp_params_validate_buffer` allows bypassing size validation via double-fetching `buffer->data_size`.
--

--- Patch [2]: [PATCH v4 2/7] media: qcom: camss: Add camss-params helper ---
commit e5eb27065c51794fadaae391d80e83b70481fa0d
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

media: qcom: camss: Add camss-params helper

Add V4L2 ISP parameter buffer validation helpers for CAMSS ISP drivers.
Provides buffer size validation and per-block type validation for the
extensible V4L2 ISP parameters buffer format.

> diff --git a/drivers/media/platform/qcom/camss/camss-params.c b/drivers/media/platform/qcom/camss/camss-params.c
> new file mode 100644
> index 0000000000000..ebb75a191d375
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-params.c
> @@ -0,0 +1,67 @@
[ ... ]
> +int camss_isp_params_apply(struct device *dev,
> +			   struct vb2_buffer *vb,
> +			   const struct v4l2_isp_params_block_type_info *type_info,
> +			   const camss_isp_params_handler_fn *handlers,
> +			   unsigned int num_handlers,
> +			   void *priv)
> +{
> +	const struct v4l2_isp_params_buffer *buf;
> +	unsigned int remaining;
> +	unsigned int offset = 0;
> +	int ret;
> +
> +	ret = v4l2_isp_params_validate_buffer_size(dev, vb,
> +					v4l2_isp_params_buffer_size(CAMSS_PARAMS_MAX_PAYLOAD));

[Severity: Low]
Where is CAMSS_PARAMS_MAX_PAYLOAD defined? The kernel build seems to fail
because this constant is undefined.

> +	if (ret)
> +		return ret;
> +
> +	buf = vb2_plane_vaddr(vb, 0);
> +
> +	ret = v4l2_isp_params_validate_buffer(dev, vb, buf, type_info, num_handlers);

[Severity: High]
Can this result in a NULL pointer dereference?

If an unprivileged userspace application queues a V4L2 buffer lacking a
virtual address mapping, vb2_plane_vaddr() will return NULL. The NULL
pointer buf is then directly passed to v4l2_isp_params_validate_buffer(),
which dereferences it.

> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "params: version=%u data_size=%u\n", buf->version, buf->data_size);
> +
> +	remaining = buf->data_size;
> +
> +	while (remaining >= sizeof(struct v4l2_isp_params_block_header)) {
> +		const union camss_isp_params_block *block =
> +			(const union camss_isp_params_block *)&buf->data[offset];
> +		u16 type  = block->header.type;
> +		u32 bsize = block->header.size;

[Severity: High]
Could this be susceptible to a double-fetch race condition?

The function validates the V4L2 parameter buffer, but then iteratively
parses it while re-fetching block->header.size from shared user memory.

If a malicious userspace thread concurrently modifies block->header.size to
0 after validation, bsize becomes 0. This means offset and remaining do not
change, which would result in an infinite kernel loop.

> +
> +		if (type < num_handlers && handlers[type])
> +			handlers[type](priv, block);
> +		else
> +			dev_dbg(dev, "params: no handler for block type %u\n", type);
> +
> +		offset += bsize;
> +		remaining -= bsize;
> +	}
[ ... ]
> diff --git a/drivers/media/platform/qcom/camss/camss-params.h b/drivers/media/platform/qcom/camss/camss-params.h
> new file mode 100644
> index 0000000000000..058e3f158b42d
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-params.h
> @@ -0,0 +1,62 @@
[ ... ]
> +#ifndef CAMSS_PARAMS_H
> +#define CAMSS_PARAMS_H
> +
> +#include <linux/types.h>
> +#include <media/v4l2-isp.h>
> +#include <uapi/linux/camss-config.h>

[Severity: Low]
Is this header correct? It appears that <uapi/linux/camss-config.h> does
not exist in the tree.

[ ... ]

While reviewing this patch, I noticed a related issue in
v4l2_isp_params_validate_buffer() in drivers/media/v4l2-core/v4l2-isp.c:

> 	/* Validate the size reported in the header */
> 	buffer_size = header_size + buffer->data_size;
> 	if (buffer_size != payload_size) {
> 		dev_dbg(dev, "Data size %zu and payload size %zu are different\n",
> 			buffer_size, payload_size);
> 		return -EINVAL;
> 	}
> 
> 	/* Walk the list of ISP configuration blocks and validate them. */
> 	buffer_size = buffer->data_size;

[Severity: High]
This is a pre-existing issue, but does this core function allow a bounds
check bypass?

The buffer->data_size field is fetched from shared user memory to validate
it fits in the payload, and then fetched a second time to initialize the
parsing loop bounds.

Can a userspace program concurrently modify buffer->data_size to a large
invalid size between the two fetches, bypassing the bounds check and
causing the block parser to read out of bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-camss-isp-ope-v4-0-51207a0319d8@oss.qualcomm.com?part=2