Re: [PATCH v7 1/6] phy: fsl-imx8mq-usb: fix typec switch leak on probe error path

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

Pre-existing issues:
- [High] A race condition during probe allows `imx_phy->tca` to be dereferenced by a Type-C consumer before it is actually assigned, leading to a NULL pointer dereference.
- [High] If the PHY device is unbound via sysfs, devm-managed structures are freed while Type-C consumers might still hold references to the switch, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v7 1/6] phy: fsl-imx8mq-usb: fix typec switch leak on probe error path ---
commit bdfb57c97d01c6ce3c92e388f5a6cf33401f0d53
Author: Felix Gu <[email protected]>

phy: fsl-imx8mq-usb: fix typec switch leak on probe error path

This commit fixes a typec switch leak that occurs on the probe error path.
It transitions the cleanup mechanism to use devm_add_action_or_reset(),
ensuring the switch is properly unregistered on both probe failure and
driver removal. The manual imx95_usb_phy_put_tca() cleanup is completely
removed.

Cc: [email protected]

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index b05d80e849a14..9a33c06d6fc39 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -266,17 +267,11 @@ static struct tca_blk *imx95_usb_phy_get_tca(struct platform_device *pdev,
>  	tca->orientation = TYPEC_ORIENTATION_NORMAL;
>  	tca->sw = tca_blk_get_typec_switch(pdev, imx_phy);

[Severity: High]
This is a pre-existing issue, but does this create a race condition where
imx_phy->tca is dereferenced before it is assigned?

When tca_blk_get_typec_switch() calls typec_switch_register(), the switch
device is immediately exposed to the system. A Type-C port controller could
discover it and call typec_switch_set(), which triggers
tca_blk_typec_switch_set().

That callback reads imx_phy->tca:

tca_blk_typec_switch_set() {
    ...
    struct tca_blk *tca = imx_phy->tca;

    if (tca->orientation == orientation)
    ...
}

However, imx_phy->tca is not assigned until imx95_usb_phy_get_tca()
completes and returns to the probe function, leaving a window where the
callback might dereference a NULL pointer.

> -	return tca;
> -}
> -
> -static void imx95_usb_phy_put_tca(struct imx8mq_usb_phy *imx_phy)
> -{
> -	struct tca_blk *tca = imx_phy->tca;
> -
> -	if (!tca)
> -		return;
> +	ret = devm_add_action_or_reset(&pdev->dev, tca_blk_put_typec_switch, tca->sw);
> +	if (ret)
> +		return ERR_PTR(ret);

[Severity: High]
This isn't a bug introduced by this patch, but does relying on devm
allocation for imx_phy and tca lead to a use-after-free if the device is
unbound?

When the PHY device is unbound via sysfs, devres automatically frees the
imx_phy and tca structures. Although typec_switch_unregister() removes the
switch from the system, Type-C consumers that already acquired references
(holding a kref via typec_switch_get()) can still invoke the switch
callback.

If a consumer calls typec_switch_set() after unbind, it will trigger
tca_blk_typec_switch_set(), which accesses the freed memory:

tca_blk_typec_switch_set() {
    struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
    struct tca_blk *tca = imx_phy->tca;
    ...
}

> -	tca_blk_put_typec_switch(tca->sw);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1