Re: [PATCH v2 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] Returning `-ENODEV` instead of `-EPROBE_DEFER` when the RPMH controller hasn't fully probed causes premature and permanent probe failures for consumers.
- [High] Missing validation of the device tree phandle target can lead to type confusion and memory corruption.

Pre-existing issues:
- [Critical] Stack variables accessed asynchronously after stack frame is destroyed on timeout, leading to Use-After-Free.
--

commit 4f9cc5ff213e0a26acf5f3c77494a0af70926c9e
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 allow any device with a pointer to
the RPMH controller device to issue write commands. It introduces
rpmh_get_ctrlr_dev() to look up the controller via a device tree phandle,
and new APIs to accept the controller device pointer directly.

> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index ca37da3dc2b14..9dbc42b775d9f 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
[ ... ]
> @@ -76,6 +77,21 @@ static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>  	return &drv->client;
>  }
>  
> +static struct rpmh_ctrlr *get_rpmh_ctrlr_from_dev(const struct device *ctrl_dev)
> +{
> +	struct rsc_drv *drv;
> +
> +	if (!ctrl_dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	drv = dev_get_drvdata(ctrl_dev);
> +
> +	if (!drv)
> +		return ERR_PTR(-ENODEV);

[Severity: High]
If the RPMH controller device is found by of_find_device_by_node() but hasn't
fully probed yet, dev_get_drvdata() will return NULL. Does returning -ENODEV
here cause a premature and permanent probe failure for consumers instead of
allowing them to gracefully defer by returning -EPROBE_DEFER?

> +
> +	return &drv->client;
> +}
[ ... ]
> +/**
> + * 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;
> +	struct device_link *link;
> +	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);

[Severity: High]
If a device tree maliciously or accidentally points the "qcom,rpmh" phandle
to a non-RPMH device, is it possible for drvdata to point to a different
driver's structure when get_rpmh_ctrlr_from_dev() evaluates it?

Would this cause type confusion and memory corruption when the consumer
subsequently casts it to struct rsc_drv? Should there be a validation
check to ensure the resolved device is actually an RPMH controller?

> +	of_node_put(rpmh_np);
> +
> +	if (!pdev)
> +		return ERR_PTR(-EPROBE_DEFER);
[ ... ]
> +/**
> + * 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: Critical]
This is a pre-existing issue, but can this lead to a stack use-after-free
if the timeout expires?

If wait_for_completion_timeout() times out, the function returns -ETIMEDOUT
and the local stack frame is destroyed. However, rpm_msg and compl are
allocated on the stack and the hardware transaction isn't explicitly
canceled.

When the hardware eventually completes the request, the interrupt handler
would call rpmh_tx_done() which accesses the stale rpm_msg->completion
pointer and calls complete(), writing to unmapped or reused stack memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-pinctrl-level-shifter-v2-0-3a6a025392bf@oss.qualcomm.com?part=1