Re: [PATCH v4 5/7] media: uapi: Add CAMSS ISP configuration definition

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The use of a `_MAX` enumerator and fixed maximum payload size macro breaks forward compatibility in an extensible UAPI.
- [High] The CAMSS driver validates and parses the V4L2 ISP parameter buffer directly from shared user memory without copying it to kernel space first, creating a severe Time-of-Check to Time-of-Use (TOCTOU) vulnerability.
- [Medium] Hardware configuration parameters explicitly documented as signed values are defined with unsigned data types.
- [Medium] The Q-format ranges documented for `camss_params_color_correct` mathematically contradict the declared 12-bit signed data type, misleading userspace developers.
- [Low] Spelling errors in the UAPI header documentation.
--

--- Patch [5]: [PATCH v4 5/7] media: uapi: Add CAMSS ISP configuration definition ---
commit 3a76aaf5e8a665b80168c10573fd48e3e63313fc
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

media: uapi: Add CAMSS ISP configuration definition

Add the uapi header camss-config.h defining the ISP parameter structures
used by the CAMSS Offline Processing Engine (OPE) driver. This includes
structures for white balance, chroma enhancement and color correction
configuration.

> diff --git a/include/uapi/linux/camss-config.h b/include/uapi/linux/camss-config.h
> new file mode 100644
> --- /dev/null
> +++ b/include/uapi/linux/camss-config.h
> @@ -0,0 +1,161 @@
[ ... ]
> +enum camss_params_block_type {
> +	CAMSS_PARAMS_WB_GAIN = 1,
> +	CAMSS_PARAMS_CHROMA_ENHAN = 2,
> +	CAMSS_PARAMS_COLOR_CORRECT = 3,
> +	CAMSS_PARAMS_MAX,
> +};

[Severity: High]
Does including CAMSS_PARAMS_MAX break forward compatibility?

If new blocks are added to the extensible V4L2 generic ISP parameter framework
in the future, the max value will change. Applications compiled against older
headers might miscalculate buffer sizes or array bounds, leading to ABI
breakage. Standard Linux UAPI guidelines recommend omitting max values for
extensible enumerations.

[ ... ]
> +/**
> + * struct camss_params_wb_gain - White Balance gains
> + *
> + * Implements the CLC_WB pipeline module.  The pipeline applies three
> + * sequential operations per channel:
> + *   1. Subtract sub-offset (black-level substraction)

[Severity: Low]
Is there a typo here? Should substraction be subtraction?

[ ... ]
> + * RGB2Y - Luma (Y) ceofficients

[Severity: Low]
Is there a typo here? Should ceofficients be coefficients?

> + * Y = v0 * R + v1 * G + v2 * B
> + *
> + * @luma_v0:  R-to-Y coefficient (12sQ8)
> + * @luma_v1:  G-to-Y coefficient (12sQ8)
> + * @luma_v2:  B-to-Y coefficient (12sQ8)
> + * @luma_k:   Y output offset    (9s,  0 = no offset)
> + *
[ ... ]
> +struct camss_params_chroma_enhan {
> +	struct v4l2_isp_params_block_header header;
> +	__u16 luma_v0;
> +	__u16 luma_v1;
> +	__u16 luma_v2;
> +	__u16 luma_k;
> +	__u16 coeff_ap;
> +	__u16 coeff_am;
> +	__u16 coeff_bp;
> +	__u16 coeff_bm;
> +	__u16 coeff_cp;
> +	__u16 coeff_cm;
> +	__u16 coeff_dp;
> +	__u16 coeff_dm;

[Severity: Medium]
Since these chroma enhancement coefficients are documented as signed 12-bit
fixed-point values (12sQ8), does defining them as unsigned __u16 force
userspace developers to cast or perform manual bitwise sign-extension logic?

Could these be defined as __s16 to allow native compiler support for
negative values?

[ ... ]
> + * @a:       G-input coefficients per output channel (12s;
> + *           a[0]=Out_G, a[1]=Out_B, a[2]=Out_R)
> + * @b:       B-input coefficients (12s)
> + * @c:       R-input coefficients (12s)
> + * @k:       per-output-channel offsets (typically 9s effective)
> + * @qfactor: Q-format selector (2u):
> + *           0 = 12sQ7  (range ~-256.0 .. +255.992)
> + *           1 = 12sQ8  (range ~-128.0 .. +127.996)
> + *           2 = 12sQ9  (range ~-64.0  .. +63.998)
> + *           3 = 12sQ10 (range ~-32.0  .. +31.999)

[Severity: Medium]
Are these documented Q-format ranges mathematically correct for a 12-bit
signed integer?

A 12-bit signed integer has a range of [-2048, 2047]. When divided by 128 for
Q7, the actual hardware range would be [-16.0, 15.992] rather than
[-256.0, 255.992], which appears to be calculated based on a 16-bit signed
integer. Could userspace developers unknowingly supply out-of-bounds parameter
values by trusting the UAPI documentation?

> + */
> +struct camss_params_color_correct {
> +	struct v4l2_isp_params_block_header header;
> +	__u16 a[3];
> +	__u16 b[3];
> +	__u16 c[3];
> +	__u16 k[3];

[Severity: Medium]
Similarly to the chroma enhancement parameters, these coefficients are
documented as 12s but are defined as unsigned __u16 fields. Should these use
a signed data type?

[ ... ]
> +#define CAMSS_PARAMS_MAX_PAYLOAD		\
> +	(sizeof(struct camss_params_wb_gain)	+\
> +	 sizeof(struct camss_params_chroma_enhan)	+\
> +	 sizeof(struct camss_params_color_correct))

[Severity: High]
Does hardcoding CAMSS_PARAMS_MAX_PAYLOAD break forward compatibility?

If new parameter blocks are added to the UAPI in future kernel releases, this
fixed maximum size will become invalid and could cause applications compiled
against older headers to miscalculate buffer sizes.

[ ... ]

While reviewing the configuration parameters, I also noticed a potential
regression in how the blocks are validated in
drivers/media/platform/qcom/camss/camss-params.c:

drivers/media/platform/qcom/camss/camss-params.c:camss_isp_params_apply() {
    ...
	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;
    ...
		offset += bsize;
		remaining -= bsize;
	}
    ...
}

[Severity: High]
Does reading bsize directly from the user-accessible memory mapping (buf->data)
introduce a Time-of-Check to Time-of-Use (TOCTOU) regression?

If a malicious userspace thread modifies bsize to 0 concurrently with the
kernel's parsing, could it cause an infinite loop and denial of service?
Similarly, if bsize is changed to a large value, remaining could underflow,
allowing offset to point arbitrarily far out-of-bounds during subsequent reads.

Does the payload need to be copied to kernel-only memory before parsing to
ensure safety?

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