Re: [PATCH v3 2/2] arm64: dts: qcom: shikra: Add CPU idle states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] irqchip/qcom-mpm: uninitialized genpd and notifier UAF in qcom_mpm_probe()
- [High] arm64: dts: qcom: shikra: invalid lock nesting and premature MPM notification
- [Medium] irqchip/qcom-mpm: cpus_in_pm counter leak on error in mpm_cpu_pm_callback()
--

commit bc4ed1a6ee147014bad47b72745f44c888126bd9
Author: Sneh Mankad <sneh.mankad@oss.qualcomm.com>

arm64: dts: qcom: shikra: Add CPU idle states

This commit adds PSCI CPU and cluster idle states for Shikra and removes
#power-domain-cells from the MPM node so the driver uses CPU PM
notifications instead of acting as a power domain provider.

> diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
> index 92d22e3b6931c..aeb608fd7180a 100644
> --- a/arch/arm64/boot/dts/qcom/shikra.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra.dtsi

[ ... ]

> @@ -334,7 +426,6 @@ mpm: interrupt-controller {
>  			mboxes = <&apcs_glb 1>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> -			#power-domain-cells = <0>;

[Severity: High]
When #power-domain-cells is removed from the mpm node, qcom_mpm_probe() in
drivers/irqchip/irq-qcom-mpm.c takes the else branch and registers the CPU
PM notifier instead of initializing genpd:

	struct generic_pm_domain *genpd;
	...
	priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
	...
	if (of_find_property(np, "#power-domain-cells", NULL)) {
		genpd = &priv->genpd;
		...
	} else {
		priv->mpm_pm.notifier_call = mpm_cpu_pm_callback;
		ret = cpu_pm_register_notifier(&priv->mpm_pm);
		if (ret) {
			dev_err(dev, "failed to register for CPU_PM notifications: %d\n", ret);
			return ret;
		}
	}

If a subsequent step in qcom_mpm_probe() fails, the error cleanup path runs:

remove_domain:
	irq_domain_remove(priv->domain);
remove_genpd:
	pm_genpd_remove(genpd);
free_mbox:
	mbox_free_channel(priv->mbox_chan);
	return ret;

If devm_request_irq() fails, execution jumps to remove_domain and falls
through to remove_genpd, passing the uninitialized stack pointer genpd to
pm_genpd_remove().

Furthermore, if irq_find_host(), irq_domain_create_hierarchy(), or
devm_request_irq() fails after cpu_pm_register_notifier(&priv->mpm_pm)
succeeds, the error path never calls
cpu_pm_unregister_notifier(&priv->mpm_pm). When devres frees priv on probe
failure, &priv->mpm_pm remains linked in the global cpu_pm_notifier chain
and will be dereferenced after free on the next CPU idle entry. And if
cpu_pm_register_notifier() itself fails, returning directly leaks
priv->mbox_chan.

Could qcom_mpm_probe() initialize genpd to NULL, guard pm_genpd_remove(),
unregister the CPU PM notifier on error, and jump to free_mbox when
cpu_pm_register_notifier() fails before switching Shikra to this path?

[Severity: Medium]
Switching the mpm node to the mpm_cpu_pm_callback() path also exposes an
error handling issue in drivers/irqchip/irq-qcom-mpm.c when
handle_rpm_notification() fails:

static int mpm_cpu_pm_callback(struct notifier_block *nfb, unsigned long action, void *v)
{
	struct qcom_mpm_priv *priv = container_of(nfb, struct qcom_mpm_priv, mpm_pm);
	int cpus_in_pm;

	switch (action) {
	case CPU_PM_ENTER:
		cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
		...
		if (cpus_in_pm < num_online_cpus())
			return NOTIFY_OK;
		break;
	case CPU_PM_ENTER_FAILED:
	case CPU_PM_EXIT:
		atomic_dec(&priv->cpus_in_pm);
		return NOTIFY_OK;
	default:
		return NOTIFY_DONE;
	}

	if (handle_rpm_notification(priv))
		return NOTIFY_BAD;

	return NOTIFY_OK;
}

During cpu_pm_enter(), raw_notifier_call_chain_robust() invokes
notifier_call_chain_robust() in kernel/notifier.c:

	ret = notifier_call_chain(nl, val_up, v, -1, &nr);
	if (ret & NOTIFY_STOP_MASK)
		notifier_call_chain(nl, val_down, v, nr-1, NULL);

When a callback returns NOTIFY_BAD, CPU_PM_ENTER_FAILED is only sent to the
nr - 1 callbacks that preceded the failing one, and cpu_pm_enter() returns
an error so cpu_pm_exit() is not called.

If handle_rpm_notification(priv) fails (for example, if mbox_send_message()
returns -ENOBUFS), wouldn't returning NOTIFY_BAD without decrementing
priv->cpus_in_pm leave priv->cpus_in_pm permanently skewed by +1?

[Severity: High]
With #power-domain-cells removed from mpm while adding both per-CPU PSCI
idle states (little-power-down, little-pll-power-down, big-power-down,
big-pll-power-down) and a cluster idle state (cluster-power-down), the
call chain on CPU idle entry becomes:

cpu_pm_enter()
  -> cpu_pm_notify_robust()
       raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
       -> mpm_cpu_pm_callback()
            -> handle_rpm_notification()
                 -> mbox_send_message()
                      -> add_to_rbuf()
                           guard(spinlock_irqsave)(&chan->lock);

Because cpu_pm_notifier.lock is a raw_spinlock_t and chan->lock in
drivers/mailbox/mailbox.c is a non-raw spinlock_t, won't acquiring
chan->lock inside mpm_cpu_pm_callback() trigger an invalid wait context bug
under CONFIG_PROVE_RAW_LOCK_NESTING and sleep in atomic context from the
idle task on PREEMPT_RT?

In addition, cpuidle-psci calls cpu_pm_enter() for every non-WFI per-CPU
idle state (via psci_enter_idle_state()) and before
pm_runtime_put_sync_suspend() determines whether cluster_pd actually enters
cluster-power-down (in __psci_enter_domain_idle_state()). Won't
mpm_cpu_pm_callback() clear MPM_REG_STATUS and notify RPM whenever all
CPUs enter shallow per-CPU idle states, even when the cluster does not
power down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-b4-shikra_lpm_addition-v3-0-424abcdcf824@oss.qualcomm.com?part=2