Re: [PATCH v3 2/3] regulator: Add RPMI voltage service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mailbox: race condition and UAF due to concurrent lockless access
- [Medium] regulator: mpxy_reg_set_voltage_sel incorrectly rejects 0V selections
--

commit 88daa08f06769cd53eb8f46c2849474d0f855277
Author: Joshua Yeong <joshua.yeong@starfivetech.com>

regulator: Add RPMI voltage service

The RPMI specification defines a voltage service group which can be
accessed via SBI MPXY extension or dedicated S-mode RPMI transport.

Add a mailbox client based regulator driver for the RISC-V RPMI voltage
service group. The driver enumerates the voltage domains advertised by
the platform microcontroller, reads their supported voltage levels and
registers each of them as a regulator.

> diff --git a/drivers/regulator/riscv-rpmi-regulator.c b/drivers/regulator/riscv-rpmi-regulator.c
> new file mode 100644
> index 0000000000000..ea230bcb6a479
> --- /dev/null
> +++ b/drivers/regulator/riscv-rpmi-regulator.c
> @@ -0,0 +1,893 @@

[ ... ]

> +static int rpmi_reg_set_config(struct rpmi_reg_domain *mpxy_reg, u32 config)
> +{
> +	struct rpmi_set_config_tx tx;
> +	struct rpmi_set_config_rx rx = { };
> +	struct rpmi_mbox_message msg;
> +	int ret;
> +
> +	tx.domain_id = cpu_to_le32(mpxy_reg->id);
> +	tx.config = cpu_to_le32(config);
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_VOLT_SRV_SET_CONFIG,
> +					  &tx, sizeof(tx), &rx, sizeof(rx));
> +
> +	ret = rpmi_mbox_send_message(mpxy_reg->rpmi_ctx->chan, &msg);
        ^^^^

[Severity: High]
Since multiple regulators instantiated by rpmi_reg_probe share the same
mpxy_ctx->chan, could concurrent regulator operations (such as from sysfs
or consumer drivers) cause a race condition here?

Because the client is set up with tx_block = false, mbox_send_message will
asynchronously queue a second message if the channel is busy and return
immediately. The rpmi_mbox_send_message helper then calls mbox_client_txdone,
forcing the queue to advance, and subsequently returns.

This destroys the stack-allocated msg variable while it is still queued,
which would lead to a use-after-free when the mailbox controller later
dereferences this dangling pointer. Is mutual exclusion needed around
these mailbox accesses?

[ ... ]

> +static int mpxy_reg_set_voltage_sel(struct regulator_dev *rdev, unsigned int selector)
> +{
> +	struct rpmi_reg_domain *mpxy_reg = rdev_get_drvdata(rdev);
> +	struct rpmi_set_level_tx tx;
> +	struct rpmi_set_level_rx rx = { };
> +	struct rpmi_mbox_message msg;
> +	s32 volt_uV;
> +	int ret;
> +
> +	volt_uV = mpxy_reg->desc.ops->list_voltage(rdev, selector);
> +	if (volt_uV <= 0)
        ^^^^

[Severity: Medium]
Will this improperly reject valid 0V operating states?

If a consumer requests a voltage mapped to a selector that evaluates to
0 uV, or the hardware advertises a range where 0V is a valid operational
level, list_voltage() will return 0. The check here returns -EINVAL for
0 uV, but 0 uV is semantically permitted in the RPMI specification and
by the core regulator APIs.

Should this check be strictly < 0 to catch errors returned by
list_voltage without rejecting valid 0V targets?

> +		return -EINVAL;
> +
> +	tx.domain_id = cpu_to_le32(mpxy_reg->id);
> +	tx.level = cpu_to_le32(volt_uV);
> +
> +	rpmi_mbox_init_send_with_response(&msg, RPMI_VOLT_SRV_SET_LEVEL,
> +					  &tx, sizeof(tx), &rx, sizeof(rx));
> +
> +	ret = rpmi_mbox_send_message(mpxy_reg->rpmi_ctx->chan, &msg);
        ^^^^

[Severity: High]
This appears to have the same lockless concurrent access issue with
a stack-allocated msg variable as rpmi_reg_set_config().

[ ... ]

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