Re: [PATCH v2] media: venus: Annotate flex arrays with __counted_by()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Applying `__counted_by` to structs that directly overlay unvalidated, untrusted firmware messages defeats compiler-based bounds checking.

Pre-existing issues:
- [High] The Venus HFI parser does not validate that the remaining buffer bytes are sufficient to hold the variable-length flexible array payloads indicated by the firmware, leading to severe out-of-bounds reads during memcpy().
- [High] Several parsing functions return an incorrect consumed payload size, causing the parser loop to desynchronize and misinterpret flexible array data as new property headers.
- [High] `parse_raw_formats()` calculates the total payload size incorrectly by assuming all elements have the same number of planes as the final element.
--

commit 475f2f7188ec43f866da689f1e919b2cf914c64a
Author: Mohammed EL Kadiri <med08elkadiri@gmail.com>

media: venus: Annotate flex arrays with __counted_by()

Add __counted_by() annotations to flexible array members in Venus HFI helper
structures. This aims to improve run-time bounds checking and compile-time
object size resolution.

> diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
> index f44059f19505a..e96d458c52fa2 100644
> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
> @@ -616,7 +616,7 @@ struct hfi_capability {
>  
>  struct hfi_capabilities {
>  	u32 num_capabilities;
> -	struct hfi_capability data[];
> +	struct hfi_capability data[] __counted_by(num_capabilities);
>  };

[Severity: High]
Does applying __counted_by() to the data array here defeat compiler-based
bounds checking?

Since num_capabilities is read directly from untrusted firmware packets without
first being validated against the actual packet allocation size (rem_bytes),
applying __counted_by() could cause the compiler to wrongly trust the
attacker-controlled count.

This isn't a bug introduced by this patch, but the Venus HFI parser does not
validate that the remaining buffer bytes are sufficient to hold the
variable-length flexible array payloads:

drivers/media/platform/qcom/venus/hfi_parser.c:hfi_parser() {
	...
		case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
			if (rem_bytes <= sizeof(struct hfi_capabilities))
				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;

			ret = parse_caps(core, codecs, domain, payload);
	...
}

drivers/media/platform/qcom/venus/hfi_parser.c:parse_caps() {
	...
	struct hfi_capabilities *caps = data;
	struct hfi_capability *cap = caps->data;
	u32 num_caps = caps->num_capabilities;
	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};

	if (num_caps > MAX_CAP_ENTRIES)
		return -EINVAL;

	memcpy(caps_arr, cap, num_caps * sizeof(*cap));
	...
}

This is a pre-existing issue, but parse_caps() returns the static header size
instead of the dynamic array size:

drivers/media/platform/qcom/venus/hfi_parser.c:parse_caps() {
	...
	return sizeof(*caps);
}

This causes the parser loop in hfi_parser() to incorrectly advance into the
middle of the flexible array data payload, leading to parsing desynchronization:

drivers/media/platform/qcom/venus/hfi_parser.c:hfi_parser() {
	...
		words += ret / sizeof(u32);
		rem_bytes -= ret;
	}
	...
}

[ ... ]

> @@ -802,7 +802,7 @@ struct hfi_profile_level {
>  
>  struct hfi_profile_level_supported {
>  	u32 profile_count;
> -	struct hfi_profile_level profile_level[];
> +	struct hfi_profile_level profile_level[] __counted_by(profile_count);
>  };

[Severity: High]
Does applying __counted_by() to the profile_level array here also defeat
compiler bounds checking for the same reasons? profile_count is read from
untrusted firmware without validation against rem_bytes.

This is a pre-existing issue, but parse_profile_level() performs an
out-of-bounds memcpy() by blindly trusting the unvalidated profile_count
field against the unchecked remainder:

drivers/media/platform/qcom/venus/hfi_parser.c:parse_profile_level() {
	struct hfi_profile_level_supported *pl = data;
	struct hfi_profile_level *proflevel = pl->profile_level;
	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};

	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
		return -EINVAL;

	memcpy(pl_arr, proflevel, pl->profile_count * sizeof(*proflevel));
	...
}

[ ... ]

> @@ -1085,7 +1085,7 @@ struct hfi_resource_ocmem_requirement {
>  
>  struct hfi_resource_ocmem_requirement_info {
>  	u32 num_entries;
> -	struct hfi_resource_ocmem_requirement requirements[];
> +	struct hfi_resource_ocmem_requirement requirements[] __counted_by(num_entries);
>  };

[Severity: High]
Can applying __counted_by() to the requirements array here lead to similar
bounds checking bypasses if num_entries is untrusted?

This isn't a bug introduced by this patch, but similar payload size
miscalculations occur in other parsing functions, such as parse_raw_formats().
The total payload size is calculated incorrectly by assuming all elements have
the same number of planes as the final element:

drivers/media/platform/qcom/venus/hfi_parser.c:parse_raw_formats() {
	...
	while (entries) {
		num_planes = pinfo->num_planes;
		...
		pinfo = (void *)pinfo + sizeof(*constr) * num_planes +
			2 * sizeof(u32);
		entries--;
	}
	...
	size = fmt->format_entries * (sizeof(*constr) * num_planes + 2 * sizeof(u32))
		+ 2 * sizeof(u32);

	return size;
}

Can this cause hfi_parser() to either advance its pointer too far or too little
if the firmware provides entries with varying plane counts?

Also a pre-existing issue, parse_alloc_mode() returns the static header size
instead of the dynamic array size:

drivers/media/platform/qcom/venus/hfi_parser.c:parse_alloc_mode() {
	struct hfi_buffer_alloc_mode_supported *mode = data;
	...
	return sizeof(*mode);
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607111933.6398-1-med08elkadiri@gmail.com?part=1