Re: [PATCH RFC v4 16/18] ACPI: RISC-V: Parse RISC-V Quality of Service Controller (RQSC) table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The parser ignores the `rqsc->num` field and may fail fatally on valid tables with trailing padding.
- [High] Unvalidated resource descriptor types may lead to misinterpretation of resource data.
- [High] The parser ignores `node->flags`, risking access to disabled or powered-down hardware.
- [High] Hardcoded resource subtable offsets create forward compatibility risks due to missing revision checks.
- [Medium] Missing validation of node `length` against the table boundary allows silent acceptance of corrupted tables.
--

commit 163a54a89d0b05cc3754e1a635d0dc2d8cbb9e84
Author: Drew Fustini <fustini@kernel.org>

ACPI: RISC-V: Parse RISC-V Quality of Service Controller (RQSC) table

Add a parser for the ACPI RQSC table, which describes the CBQRI
controllers in a system. For each table entry, populate a
cbqri_controller_info descriptor and hand it to the CBQRI driver via
riscv_cbqri_register_controller().

Link: https://github.com/riscv-non-isa/riscv-rqsc/blob/main/src/
Link: https://github.com/riscv-non-isa/riscv-cbqri/releases/tag/v1.0

> diff --git a/drivers/acpi/riscv/rqsc.c b/drivers/acpi/riscv/rqsc.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/acpi/riscv/rqsc.c
[ ... ]
> +	for (node = ACPI_ADD_PTR(struct acpi_rqsc_node, rqsc,
> +				 sizeof(struct acpi_table_rqsc));
> +	     node < end;
> +	     node = ACPI_ADD_PTR(struct acpi_rqsc_node, node, node->length)
> +	) {
> +		const struct acpi_rqsc_resource *res0;
> +		struct cbqri_controller_info info = {};
> +		int ret;
> +
> +		if ((void *)node + sizeof(*node) > (void *)end) {
> +			pr_err("truncated entry at end of table, aborting\n");
> +			riscv_cbqri_unregister_last(num_controllers);
> +			return -EINVAL;
> +		}

Does this code also need to verify that the entire declared length of the node
fits within the table boundary?

If a malformed node declares a length extending beyond the end of the ACPI
table, the node pointer could advance past end during the loop increment:

    node = ACPI_ADD_PTR(struct acpi_rqsc_node, node, node->length)

Could this cleanly fail the loop condition on the next iteration and silently
return success instead of rejecting the corrupted table?

> +
> +		if (node->length < sizeof(*node)) {
> +			pr_err("malformed RQSC entry: length %u < %zu, aborting\n",
> +			       node->length, sizeof(*node));
> +			riscv_cbqri_unregister_last(num_controllers);
> +			return -EINVAL;
> +		}

Could this loop be bounded by rqsc->num instead of just node < end?

If the firmware pads the table at the end for alignment, the parser might
attempt to process the zeroed padding as a node.

Would encountering this padding trigger this length check, unregister all
successfully parsed controllers, and completely abort initialization for an
otherwise valid table?

> +
> +		/* GAS must describe system memory. ioremap() consumes it later. */
> +		if (node->reg.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +			pr_warn("controller has unsupported address space_id=%u, skipping\n",
> +				node->reg.space_id);
> +			continue;
> +		}

Should this code check node->flags before proceeding? Standard ACPI practice
typically uses Bit 0 to indicate whether a hardware block is present and
enabled.

If flags are ignored, could the driver risk mapping and accessing
memory-mapped registers of explicitly disabled hardware?

[ ... ]
> +		/*
> +		 * Resources follow the node header in-line. Only res[0] is
> +		 * consumed. Bound it against end before reading its prefix so
> +		 * a table that ends partway through a resource subtable is
> +		 * rejected rather than read past the mapping.
> +		 */
> +		res0 = (const struct acpi_rqsc_resource *)
> +		       ((const u8 *)node + sizeof(*node));

Is it safe to hardcode the start of the resource subtables using
sizeof(*node)?

If a future revision of the RQSC table extends the acpi_rqsc_node structure
with additional fields, sizeof(*node) would be smaller than the actual
node header size.

Could this missing rqsc->header.revision check cause the parser to point into
the middle of the new header fields and incorrectly interpret them as the
first resource descriptor?

[ ... ]
> +		/*
> +		 * id1 is u64 on the wire but cache_id and prox_dom are u32
> +		 * downstream (PPTT cache_id, ACPI proximity domain). Reject
> +		 * rather than truncate, so a too-large id is not silently
> +		 * mapped to the wrong PPTT entry or NUMA node.
> +		 */
> +		if (res0->id1 > U32_MAX) {
> +			pr_warn("controller at %pa: id1 0x%llx exceeds u32, skipping\n",
> +				&info.addr, res0->id1);
> +			continue;
> +		}
> +
> +		switch (info.type) {
> +		case CBQRI_CONTROLLER_TYPE_CAPACITY:
> +			info.cache_id = (u32)res0->id1;
> +			break;
> +		case CBQRI_CONTROLLER_TYPE_BANDWIDTH:
> +			info.prox_dom = (u32)res0->id1;
> +			break;

Does this code need to validate the type and id_type fields of the resource
descriptor before extracting res0->id1?

If a controller specifies multiple resources (node->nres > 1) and the first
descriptor is not the expected cache ID or proximity domain, could the
parser blindly misinterpret its contents?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-ssqosid-cbqri-rqsc-v7-0-v4-0-eb53831ef683@kernel.org?part=16