Re: [PATCH 2/2] pmdomain: riscv: Add RPMI device power service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing cleanup of initialized PM domains on the probe error path leads to a Use-After-Free in the global `gpd_list`.
- [High] Concurrent mailbox accesses cause stack Use-After-Free and queue corruption due to un-synchronized, non-blocking requests.
- [High] Ignoring the return value of `pm_genpd_remove()` in the driver remove path leaves dangling pointers in the global `gpd_list`.
- [Medium] Missing little-endian to CPU conversions on RPMI RX message fields causes data corruption on big-endian platforms.
- [Medium] Domains initialized in the ON state will never be powered off because the driver lacks a `sync_state` callback.
- [Medium] Uninitialized stack memory in RPMI response structures can be exposed to user-space.
--

commit 0568e3713c09ba7a392adb765c276307e1fd7322
Author: Joshua Yeong <joshua.yeong@starfivetech.com>

pmdomain: riscv: Add RPMI device power service

Add a generic power domain provider on top of the RISC-V Platform
Management Interface (RPMI) device power service group, reached through
an SBI MPXY channel.

> diff --git a/drivers/pmdomain/riscv/riscv-rpmi-device-power.c b/drivers/pmdomain/riscv/riscv-rpmi-device-power.c
> new file mode 100644
> index 0000000000000..f34ef48dff394
> --- /dev/null
> +++ b/drivers/pmdomain/riscv/riscv-rpmi-device-power.c
> @@ -0,0 +1,441 @@
[ ... ]
> +static int rpmi_power_state_get(struct rpmi_device_power_domain *mpxy_pm_domain,
> +				u32 domain_id, u32 *state)
> +{
> +	struct rpmi_pm_get_power_state_tx tx;
> +	struct rpmi_pm_get_power_state_rx rx;
> +	struct rpmi_mbox_message msg;
> +	int ret;
> +
> +	tx.domain_id = cpu_to_le32(domain_id);
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_DP_SRV_GET_STATE,
> +					  &tx, sizeof(tx), &rx, sizeof(rx));
> +
> +	ret = rpmi_mbox_send_message(mpxy_pm_domain->mpxy_ctx->chan, &msg);
> +	if (ret)
> +		return ret;
> +
> +	if (rx.status)
> +		return rpmi_to_linux_error(rx.status);
> +
> +	*state = rx.power_state;

[Severity: Medium]
Will this return corrupted state data on big-endian platforms?

The driver correctly uses cpu_to_le32() for TX fields but fails to use
le32_to_cpu() when reading the raw little-endian power_state field from the
RPMI RX message.

> +
> +	return ret;
> +}
> +
> +static int rpmi_power_state_set(struct rpmi_device_power_domain *mpxy_pm_domain,
> +				u32 domain_id, u32 state)
> +{
> +	struct rpmi_pm_set_power_state_tx tx;
> +	struct rpmi_pm_set_power_state_rx rx;
> +	struct rpmi_mbox_message msg;
> +	int ret;
> +
> +	tx.domain_id = cpu_to_le32(domain_id);
> +	tx.power_state = cpu_to_le32(state);
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_DP_SRV_SET_STATE,
> +					  &tx, sizeof(tx), &rx, sizeof(rx));
> +	ret = rpmi_mbox_send_message(mpxy_pm_domain->mpxy_ctx->chan, &msg);

[Severity: High]
Can concurrent mailbox accesses cause stack Use-After-Free and queue
corruption?

Because the mailbox is configured for non-blocking execution in probe
(tx_block = false), mbox_send_message() will asynchronously queue messages if
the channel is busy. Since msg is stack-allocated, returning from
rpmi_power_state_set() destroys the message structure while it may still be
in the mailbox queue.

> +	if (ret)
> +		return ret;
[ ... ]
> +static int rpmi_device_power_get_attrs(u32 domain_id,
> +				       struct rpmi_device_power_domain *mpxy_pm_domain)
> +{
> +	struct rpmi_pm_get_domain_attrs_tx tx;
> +	struct rpmi_pm_get_domain_attrs_rx rx;
> +	struct rpmi_mbox_message msg;
> +	int ret;
> +
> +	tx.domain_id = cpu_to_le32(domain_id);
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_DP_SRV_GET_ATTRS,
> +					  &tx, sizeof(tx), &rx, sizeof(rx));
> +	ret = rpmi_mbox_send_message(mpxy_pm_domain->mpxy_ctx->chan, &msg);
> +	if (ret)
> +		return ret;
> +
> +	if (rx.status)
> +		return rpmi_to_linux_error(rx.status);
> +
> +	mpxy_pm_domain->transition_latency = rx.transition_latency;

[Severity: Medium]
Is an endianness conversion missing here?

The transition latency field is read directly without little-endian to CPU
conversion, which could cause incorrect latency evaluations on big-endian
systems.

> +	strscpy(mpxy_pm_domain->name, rx.name, RPMI_PM_DOMAIN_NAME_LEN);

[Severity: Medium]
Does unconditionally copying rx.name here expose uninitialized stack memory
to user-space?

Because the rx struct is uninitialized on the stack, if the RPMI firmware
returns a success status but populates fewer bytes than the expected size,
uninitialized stack memory will be copied into the PM domain structure and
subsequently exposed through sysfs or debugfs.

> +
> +	return 0;
> +}
[ ... ]
> +static int rpmi_pm_domain_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct rpmi_device_power_domain *mpxy_pd;
> +	struct genpd_onecell_data *mpxy_pd_data;
> +	struct generic_pm_domain **domains;
> +	struct device *dev = &pdev->dev;
> +	struct rpmi_ctx *mpxy_ctx;
> +	u32 num_domains = 0;
> +	int ret;
> +	u32 i;
> +
> +	mpxy_ctx = devm_kzalloc(&pdev->dev, sizeof(*mpxy_ctx), GFP_KERNEL);
> +	if (!mpxy_ctx)
> +		return -ENOMEM;
> +
> +	/* Setup mailbox client */
> +	mpxy_ctx->client.dev		= dev;
> +	mpxy_ctx->client.rx_callback	= NULL;
> +	mpxy_ctx->client.tx_block	= false;
> +	mpxy_ctx->client.knows_txdone	= true;

[Severity: High]
Does configuring the mailbox client for non-blocking execution
(tx_block = false) require driver-level serialization?

Without synchronization, concurrent state change requests from different
devices will queue stack-allocated messages and cause a Use-After-Free when
the calling functions return immediately.

> +	mpxy_ctx->client.tx_tout	= 0;
[ ... ]
> +	mpxy_pd_data->domains = domains;
> +	mpxy_pd_data->num_domains = num_domains;
> +
> +	platform_set_drvdata(pdev, mpxy_pd_data);
> +
> +	return of_genpd_add_provider_onecell(np, mpxy_pd_data);
> +}

[Severity: High]
If of_genpd_add_provider_onecell() fails, does returning the error directly
leave initialized PM domains dangling in the global gpd_list?

The framework does not clean up individual initialized domains when provider
registration fails. When devres eventually frees the backing memory, the
global gpd_list will contain dangling pointers, leading to a Use-After-Free
and kernel panic.

> +
> +static void rpmi_pm_domain_remove(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct genpd_onecell_data *mpxy_pd_data;
> +	unsigned int i;
> +
> +	of_genpd_del_provider(np);
> +	mpxy_pd_data = platform_get_drvdata(pdev);
> +	for (i = 0; i < mpxy_pd_data->num_domains; i++) {
> +		if (!mpxy_pd_data->domains[i])
> +			continue;
> +		pm_genpd_remove(mpxy_pd_data->domains[i]);

[Severity: High]
Is it safe to ignore the return value of pm_genpd_remove() here?

If the driver is unbound via sysfs while consumer devices are still attached,
pm_genpd_remove() will fail and return -EBUSY. Ignoring this failure allows
the function to complete and devres to free the memory while the domains are
still actively linked in the global list, causing a Use-After-Free.

> +	}
> +}
> +
> +static const struct of_device_id rpmi_pm_domain_of_match[] = {
> +	{ .compatible = "riscv,rpmi-device-power" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, rpmi_pm_domain_of_match);
> +
> +static struct platform_driver rpmi_pm_domain_platdrv = {
> +	.driver = {
> +		.name = "riscv-rpmi-device-power",
> +		.of_match_table = rpmi_pm_domain_of_match,
> +	},
> +	.probe = rpmi_pm_domain_probe,
> +	.remove = rpmi_pm_domain_remove,
> +};

[Severity: Medium]
Will domains initialized in the ON state remain powered on forever?

The platform driver omits a .sync_state callback. Without this callback (or
setting GENPD_FLAG_NO_STAY_ON), the genpd core sets stay_on = true for
default-ON domains and never clears it, preventing them from being powered
off.

> +
> +module_platform_driver(rpmi_pm_domain_platdrv);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829205520.1691-1-joshua.yeong@starfivetech.com?part=2