Re: [PATCH v4 3/3] firmware: arm_scmi: Pre-register protocol, name tupples for IMX protocols

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] SCMI bus initialization fails and aborts entirely when SCMI RAW mode is enabled without COEX.
- [High] Adding vendor-specific protocol IDs to the global `scmi_std_id_table` causes cross-vendor namespace collisions and type confusion on non-IMX platforms.
- [Medium] Dynamic allocations for SCMI requested devices are leaked on module initialization failure and module unload.
--

commit 80137af0c1515baef478cde4cc26837986f858f7
Author: Hans de Goede <johannes.goede@oss.qualcomm.com>

firmware: arm_scmi: Pre-register protocol, name tupples for IMX protocols

This patch pre-registers IMX vendor protocols to enable module auto-loading.
I've been reviewing this patchset and noticed a few regressions related to
these changes.

> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 111727904a892..9f5740dc606fa 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
[ ... ]
> @@ -594,6 +595,12 @@ static const struct scmi_device_id scmi_std_id_table[] = {
>  	{ SCMI_PROTOCOL_POWERCAP, "powercap" },
>  	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
>  	{ SCMI_PROTOCOL_PINCTRL, "pinctrl-imx" },
> +	/* Vendor protocols with in tree drivers */
> +	{ SCMI_PROTOCOL_IMX_BBM, "imx-bbm-key" },
> +	{ SCMI_PROTOCOL_IMX_BBM, "imx-bbm-rtc" },

[Severity: High]
By adding vendor-specific protocol IDs (like SCMI_PROTOCOL_IMX_BBM, which
maps to 0x81) to the global scmi_std_id_table, any SCMI platform
(e.g., Mediatek) that implements its own custom vendor protocol 0x81
will now trigger the creation of an imx-bbm-key SCMI device during boot.

This causes the NXP/IMX driver to probe and request protocol ops for 0x81.

Because the SCMI core looks up protocols by the platform's vendor_id, it
returns the other vendor's protocol operations struct (as a void *).

The IMX driver then casts this to struct scmi_imx_bbm_proto_ops * and
dereferences it.

Could this lead to arbitrary function pointer execution or memory corruption
on non-IMX platforms?


I also noticed a few other regressions introduced in this patchset regarding
the scmi_bus_init() changes.

[Severity: High]
When CONFIG_ARM_SCMI_RAW_MODE_SUPPORT is enabled and
CONFIG_ARM_SCMI_RAW_MODE_SUPPORT_COEX is disabled,
scmi_protocol_device_request() unconditionally returns -EINVAL.

Because this patchset moved the global
scmi_protocol_table_register(scmi_std_id_table) call into scmi_bus_init(),
the -EINVAL error now propagates up:

drivers/firmware/arm_scmi/bus.c:scmi_bus_init() {
    ...
	retval = scmi_protocol_table_register(scmi_std_id_table);
	if (retval) {
		bus_unregister(&scmi_bus_type);
		return retval;
	}
    ...
}

Will this cause the entire SCMI bus initialization to fail and abort when
SCMI RAW mode is enabled without COEX?


[Severity: Medium]
The scmi_bus_init() function dynamically allocates entries in the
scmi_requested_devices IDR via scmi_protocol_table_register().

However, scmi_bus_exit() omits the call to
scmi_protocol_table_unregister(scmi_std_id_table) to free these entries:

drivers/firmware/arm_scmi/bus.c:scmi_bus_exit() {
	/*
	 * Destroy all remaining devices: just in case the drivers were
	 * manually unbound and at first and then the modules unloaded.
	 */
	scmi_devices_unregister();
	bus_unregister(&scmi_bus_type);
	ida_destroy(&scmi_bus_id);
}

Furthermore, if scmi_protocol_table_register() fails halfway through (e.g.
due to -ENOMEM), it does not clean up the already allocated entries.

Are the struct scmi_requested_dev allocations leaking on module unload or
initialization failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826102734.31766-1-johannes.goede@oss.qualcomm.com?part=3