Re: [PATCH RFC 06/10] firmware: riscv: rpmi-tee: parse system information tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] firmware: riscv: rpmi-tee: forward compatibility broken by rejecting unknown tables and flags
- [Medium] firmware: riscv: rpmi-tee: parser improperly rejects omitted SERVICE table
--

commit e7385de8b0fcda79d633aac804a487469dcd9e42
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

firmware: riscv: rpmi-tee: parse system information tables

This commit adds parsing for the RPMI TEE descriptor-table SYSINFO format
used during discovery. It validates the headers, table directory, endpoint
role flags, and service mappings for both Self and Whole-system responses.

> diff --git a/drivers/firmware/riscv_rpmi_tee/sysinfo.c b/drivers/firmware/riscv_rpmi_tee/sysinfo.c
> new file mode 100644
> index 0000000000000..5829ab6db0426
> --- /dev/null
> +++ b/drivers/firmware/riscv_rpmi_tee/sysinfo.c

[ ... ]

> +	switch (type) {

[ ... ]

> +	case RPMI_TEE_SYSINFO_TABLE_BLOB:
> +		/* BLOB table is defined as raw bytes. */
> +		if (desc_size != 1)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}

[ ... ]

> +	info->data = data;
> +	info->len = len;
> +	for (i = 0; i < table_count; i++) {
> +		const struct rpmi_tee_sysinfo_table_desc *desc;
> +		int ret;
> +
> +		desc = rpmi_tee_sysinfo_desc_at(header, i);
> +		ret = rpmi_tee_sysinfo_get_table(info, desc);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If a future firmware update introduces a new, unrecognized table type,
rpmi_tee_sysinfo_get_table() falls through to the default case and returns
-EOPNOTSUPP, which causes rpmi_tee_sysinfo_init() to completely abort
initialization.

Since desc_size validation handles minor version size extensions, should
unknown table types be safely ignored here instead to maintain forward
compatibility with newer firmware revisions?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +/* Validate endpoint records and their service ranges. */
> +static int rpmi_tee_sysinfo_validate_endpoints(struct rpmi_tee_sysinfo *info,
> +					       bool require_single_ree,
> +					       u32 *self_ep_id)
> +{
> +	const struct rpmi_tee_sysinfo_table *service_table;
> +	const struct rpmi_tee_sysinfo_table *ep_table;
> +	const struct rpmi_tee_sysinfo_endpoint *ep;
> +	u32 ree_count = 0, ep_idx;
> +
> +	ep_table = &info->tables[RPMI_TEE_SYSINFO_TABLE_ENDPOINT];
> +	service_table = &info->tables[RPMI_TEE_SYSINFO_TABLE_SERVICE];
> +	/* Endpoint table should exist and not empty; service table can be empty. */
> +	if (!ep_table->present || !ep_table->count || !service_table->present)
> +		return -EINVAL;

[Severity: Medium]
The comment directly above notes that the service table can be empty, but
this condition returns -EINVAL if !service_table->present.

If valid firmware omits the service table descriptor entirely (e.g., when no
services are available to save directory space), will this spuriously reject
a fully compliant SYSINFO response?

> +
> +	for (ep_idx = 0; ep_idx < ep_table->count; ep_idx++) {
> +		rpmi_tee_sysinfo_entry_at(ep_table, ep_idx, ep);
> +
> +		/* struct rpmi_tee_sysinfo_endpoint. */
> +		u32 ep_id = get_unaligned_le32(&ep->id);
> +		u32 parent_id = get_unaligned_le32(&ep->parent_id);
> +		u32 ep_flags = get_unaligned_le32(&ep->flags);
> +		u32 service_first = get_unaligned_le32(&ep->service_first);
> +		u32 service_range_count =
> +			get_unaligned_le32(&ep->service_count);
> +
> +		bool is_physical =
> +			ep_flags & RPMI_TEE_SYSINFO_ENDPOINT_F_PHYSICAL;
> +		bool is_ree = ep_flags & RPMI_TEE_SYSINFO_ENDPOINT_F_REE;
> +		/* Require exactly one endpoint location and security role. */
> +		if (is_physical ==
> +			    !!(ep_flags & RPMI_TEE_SYSINFO_ENDPOINT_F_PROXIED) ||
> +		    is_ree == !!(ep_flags & RPMI_TEE_SYSINFO_ENDPOINT_F_TEE) ||
> +		    ep_flags & ~RPMI_TEE_SYSINFO_ENDPOINT_F_MASK)
> +			return -EINVAL;

[Severity: Medium]
Similar to the unknown tables issue above, the strict check against
~RPMI_TEE_SYSINFO_ENDPOINT_F_MASK completely rejects endpoints if they
set newly introduced flags.

Could this strict mask validation cause older kernels to fail to initialize
when running on newer firmware that utilizes new bits in ep_flags?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-riscv-rpmi-tee-abi-v1-0-04908b81d885@oss.qualcomm.com?part=6