Re: [PATCH 1/4] soc: qcom: rpmh: Allow non-child devices to issue write commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Casting `put_device` to `void (*)(void *)` for `devm_add_action_or_reset` will trigger a Control-Flow Integrity (CFI) kernel panic.
- [High] Lack of device links allows non-child consumer devices to access the hardware after unbind or while suspended, leading to UAF or physical state machine violations.

Pre-existing issues:
- [High] If `wait_for_completion_timeout` times out, a dangling stack pointer is left for the hardware completion interrupt to access, causing stack corruption.
--

commit 033be046152076a7be483616e27b4cc044eaf8da
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

soc: qcom: rpmh: Allow non-child devices to issue write commands

This patch extends the RPMH driver to support write commands from any
device that has a pointer to the RPMH controller device by looking up
the controller via the qcom,rpmh device tree phandle.

> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index ca37da3dc2b14..9c7844434e9a1 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c

[ ... ]

> @@ -271,6 +294,114 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
>  }
>  EXPORT_SYMBOL_GPL(rpmh_write);
>  
> +/**
> + * rpmh_get_ctrlr_dev: Get RPMH controller device from device tree
> + *
> + * @dev: Device with "qcom,rpmh" phandle property
> + *
> + * Returns: Pointer to RPMH controller device, with a devm action registered
> + * on @dev to release the reference when @dev is unbound.
> + */
> +struct device *rpmh_get_ctrlr_dev(struct device *dev)
> +{
> +	struct device_node *rpmh_np;
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	rpmh_np = of_parse_phandle(dev->of_node, "qcom,rpmh", 0);
> +	if (!rpmh_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	pdev = of_find_device_by_node(rpmh_np);
> +	of_node_put(rpmh_np);
> +
> +	if (!pdev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +

[Severity: High]
Since qcom,rpmh is a custom property, fw_devlink does not automatically
establish a device link. Without a device link created here (e.g., via
device_link_add()), is there any guarantee on PM or probe ordering?

Without a device link, a consumer could access the RPMH hardware while it is
runtime-suspended or after the RPMH driver has been unbound. If the RPMH
driver unbinds, its devres-managed struct rsc_drv is freed. The consumer's
devm action only pins the struct device, so calling dev_get_drvdata() later
could return a dangling pointer, leading to a Use-After-Free.

> +	ret = devm_add_action_or_reset(dev, (void (*)(void *))put_device,
> +				       &pdev->dev);

[Severity: High]
Will casting put_device to (void (*)(void *)) trigger a Control-Flow
Integrity (CFI) kernel panic?

The put_device function is defined as void put_device(struct device *dev).
Under KCFI, the mismatch between the expected function signature and the
actual signature of put_device will fail the hash check, resulting in a
kernel panic when the consumer device unbinds or its probe fails.

Could a wrapper function matching the void (*)(void *) signature be used
instead?

> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return &pdev->dev;
> +}
> +EXPORT_SYMBOL_GPL(rpmh_get_ctrlr_dev);

[ ... ]

> +/**
> + * rpmh_write_ctrlr: Write RPMH commands and block until response,
> + * with the controller device pointer
> + *
> + * @ctrlr_dev: The RPMH controller device
> + * @state: Active/sleep set
> + * @cmd: The payload data
> + * @n: The number of elements in @cmd
> + *
> + * May sleep. Do not call from atomic contexts.
> + */
> +int rpmh_write_ctrlr(const struct device *ctrlr_dev, enum rpmh_state state,
> +		     const struct tcs_cmd *cmd, u32 n)
> +{
> +	DECLARE_COMPLETION_ONSTACK(compl);
> +	/* dev is unused in the synchronous non-batch path; pass NULL */
> +	DEFINE_RPMH_MSG_ONSTACK(NULL, state, &compl, rpm_msg);
> +	struct rpmh_ctrlr *ctrlr;
> +	int ret;
> +
> +	ctrlr = get_rpmh_ctrlr_from_dev(ctrlr_dev);
> +	if (IS_ERR(ctrlr))
> +		return PTR_ERR(ctrlr);
> +
> +	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n);
> +	if (ret)
> +		return ret;
> +
> +	ret = __rpmh_write_direct(ctrlr, state, &rpm_msg);
> +	if (ret)
> +		return ret;
> +
> +	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
> +	WARN_ON(!ret);
> +	return (ret > 0) ? 0 : -ETIMEDOUT;

[Severity: High]
This isn't a bug introduced by this patch, but it appears to be a
pre-existing issue copied from rpmh_write(). If
wait_for_completion_timeout() expires, doesn't it leave a dangling stack
pointer for the hardware completion interrupt to access?

The rpm_msg and its completion are allocated on the stack. If the timeout
expires, the function returns and its stack frame is released, but the
hardware request is not canceled.

When the hardware eventually completes the transaction, the asynchronous
interrupt handler calls rpmh_tx_done(), which dereferences the now-invalid
rpm_msg pointer and attempts to call complete(), corrupting stack memory.

> +}
> +EXPORT_SYMBOL_GPL(rpmh_write_ctrlr);
> +
>  static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-pinctrl-level-shifter-v1-0-1965461d0a7c@oss.qualcomm.com?part=1