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

On Tue, May 12, 2026 at 09:46:28PM +0000, [email protected] wrote:
> > +	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?

I will add an explicit check so that an overrun is flagged as corruption
rather than silently truncating the controller list.

> > +		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?

The behavior is intentional. RQSC does not document a padding allowance,
so a zero length entry inside the declared table length is treated as
corruption.

> > +		/* 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?

Ignoring node->flags is correct here as RQSC defines bit 0 as: "When
set, indicates the controller supports allocating zero capacity blocks
or zero bandwidth blocks to an RCID."

> > +		/*
> > +		 * 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?

I will add a revision check at the top of acpi_parse_rqsc()

> > +		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?

I will add a check for res0->type and res0->id_type before using id1.

-Drew