Re: [PATCH v11 4/9] media: uapi: Add CAMSS ISP configuration definition

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: uapi: 1-based camss_params_block_type breaks v4l2-isp validation
--

commit 5063b494328e812916323b381b136d2e5d3ac987
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

media: uapi: Add CAMSS ISP configuration definition

This commit adds the UAPI header qcom-camss-config.h defining the ISP
parameter structures and shared block type identifiers for the CAMSS
Offline Processing Engine (OPE) driver.

> diff --git a/include/uapi/linux/qcom-camss-config.h b/include/uapi/linux/qcom-camss-config.h
> new file mode 100644
> index 0000000000000..5756d165e7e87
> --- /dev/null
> +++ b/include/uapi/linux/qcom-camss-config.h
> @@ -0,0 +1,212 @@
> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> +/*
> + * Qualcomm CAMSS ISP Driver - Userspace API
> + *
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#ifndef _UAPI_LINUX_QCOM_CAMSS_CONFIG_H
> +#define _UAPI_LINUX_QCOM_CAMSS_CONFIG_H
> +
> +#include <linux/types.h>
> +#include <linux/media/v4l2-isp.h>
> +
> +/**
> + * enum camss_params_block_type - CAMSS ISP parameter block identifiers
> + *
> + * Each value identifies one ISP processing block.  The value is placed in
> + * the @type field of &struct v4l2_isp_block_header.
> + *
> + * @CAMSS_PARAMS_WB_GAIN: white balance gains and offsets (CLC_WB),
> + *	&struct camss_params_wb_gain
> + * @CAMSS_PARAMS_CHROMA_ENHAN: RGB to YUV colour transfer matrix
> + *	(CLC_CHROMA_ENHAN), &struct camss_params_chroma_enhan
> + * @CAMSS_PARAMS_COLOR_CORRECT: colour correction matrix (CLC_CC),
> + *	&struct camss_params_color_correct
> + * @CAMSS_PARAMS_GAMMA: per-channel gamma correction curves (CLC_GLUT),
> + *	&struct camss_params_gamma
> + */
> +enum camss_params_block_type {
> +	CAMSS_PARAMS_WB_GAIN = 1,
> +	CAMSS_PARAMS_CHROMA_ENHAN = 2,
> +	CAMSS_PARAMS_COLOR_CORRECT = 3,
> +	CAMSS_PARAMS_GAMMA = 4,
> +};

[Severity: High]
Starting enum camss_params_block_type at 1 (and sharing the enum across
ISPs that may only populate a subset of blocks in their
v4l2_isp_params_block_type_info array) leaves index 0 and any unsupported
block indices zero-initialized with info->size == 0.

In drivers/media/v4l2-core/v4l2-isp.c:v4l2_isp_params_validate_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->block_validate && info->block_validate(dev, block))
			return -EINVAL;

		block_offset += block->size;
		buffer_size -= block->size;

If userspace queues a parameter buffer with data_size >=
sizeof(struct v4l2_isp_block_header) containing a zeroed block header
(type = 0, flags = 0, size = 0), block->type passes the bounds check and
block->size != info->size evaluates 0 != 0 (false). Because block->size is
0, block_offset and buffer_size never advance in the validation loop,
causing an infinite loop in the kernel.

Additionally, if userspace submits block->type = 0 with
V4L2_ISP_PARAMS_FL_BLOCK_DISABLE and block->size == sizeof(*block),
v4l2_isp_params_validate_buffer() accepts the unpopulated block type 0 as
valid.

Should enum camss_params_block_type start at 0 like other V4L2 ISP block
type enums, or should v4l2_isp_params_validate_buffer() also reject
entries where info->size == 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-camss-isp-ope-v11-0-0c2937195298@oss.qualcomm.com?part=4