Re: [PATCH v2 1/4] phy: qcom: qmp-combo: skip USB power_off/exit after device teardown

[sashiko-bot@kernel.org]

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

New issues:
- [High] The lockless check of `qmp->usb_init_count` in `qmp_combo_usb_exit()` introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition that can lead to an integer underflow and unbalanced resource management.

Pre-existing issues:
- [Critical] The patch attempts to prevent a crash by checking `qmp->usb_init_count` after the device's backing resources are released, but the `qmp` structure itself has already been freed via `devm` cleanup, resulting in a Use-After-Free.
--

commit 639386ffdb78db9a5c3e78a1f3ada8713f38acd7
Author: Michael Scott <mike.scott@oss.qualcomm.com>

phy: qcom: qmp-combo: skip USB power_off/exit after device teardown

The commit attempts to prevent crashes when external consumers like dwc3
call phy_exit() after the QMP PHY's backing resources have been released,
by guarding the power_off and exit paths with a check on the
usb_init_count balance.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index cdcfad2e86b1d..0db200292642e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3926,6 +3926,17 @@ static int qmp_combo_usb_power_off(struct phy *phy)
>  	struct qmp_combo *qmp = phy_get_drvdata(phy);
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  
> +	/*
> +	 * Reachable as ->exit from external consumers (notably dwc3) after
> +	 * this device's backing resources have already been released along
> +	 * a teardown chain. Refuse to touch registers in that case.
> +	 */
> +	if (!qmp->usb_init_count) {

[Severity: Critical]
This is a pre-existing issue, but the qmp structure itself is allocated
using devm_kzalloc(), so it is freed during the device's devres cleanup.
If dwc3 calls phy_exit() after qmp_combo has been unbound, won't accessing
qmp->usb_init_count result in a Use-After-Free (UAF) read?

If the freed memory happens to be zeroed, the read evaluates to 0 and
successfully masks the bug by skipping the remainder of the function. If
SLUB poisoning or KASAN is enabled, this will trigger a UAF splat or read a
non-zero value, proceeding to call mutex_lock(&qmp->phy_mutex) on freed
memory.

Does this patch replace a vmap UAF on qmp->pcs with a slab UAF on the freed
qmp structure instead of fixing the underlying lifecycle bug?

[ ... ]

> @@ -3968,6 +3979,17 @@ static int qmp_combo_usb_exit(struct phy *phy)
>  	struct qmp_combo *qmp = phy_get_drvdata(phy);
>  	int ret;
>  
> +	/*
> +	 * See qmp_combo_usb_power_off(): an external consumer may call
> +	 * phy_exit() after the QMP device's resources have been torn
> +	 * down. usb_init_count tracks usb_init/usb_exit balance.
> +	 */
> +	if (!qmp->usb_init_count) {

[Severity: High]
Does checking qmp->usb_init_count outside of qmp->phy_mutex introduce a
Time-of-Check to Time-of-Use (TOCTOU) race?

If a concurrent thread (such as qmp_combo_typec_mux_set() reacting to a
Type-C event) acquires the mutex and decrements usb_init_count to zero
immediately after this lockless check succeeds, qmp_combo_usb_exit() will
proceed to acquire the lock. 

It will then call qmp_combo_usb_power_off(), which will safely check the
count under the lock and return 0 for success.

Because qmp_combo_usb_power_off() returns success, qmp_combo_usb_exit()
mistakenly proceeds to call qmp_combo_com_exit() and unconditionally
executes qmp->usb_init_count--. 

Since the counter is an unsigned int and is already 0, could this wrap
around to UINT_MAX and cause unbalanced regulator and clock disables
which can crash the system?

> +		dev_dbg(qmp->dev, "%s: PHY not initialised, skipping\n",
> +			__func__);
> +		return 0;
> +	}
> +
>  	mutex_lock(&qmp->phy_mutex);
>  	ret = qmp_combo_usb_power_off(phy);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521010935.1333494-1-mike.scott@oss.qualcomm.com?part=1