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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Potential NULL pointer dereference if the Type-C switch's `.set()` callback is invoked concurrently during probe.
- [High] Potential Use-After-Free if a typec port driver holds a reference to the switch and calls `.set()` after the PHY driver unbinds.
--

commit 546b07c54e0077d325cf3fd20b1dd95d1c123a9d
Author: Felix Gu <ustc.gu@gmail.com>

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

If probe fails after imx95_usb_phy_get_tca() succeeds, the typec
switch leaks because the only cleanup path was in .remove, which
never runs on probe failure.

Use devm_add_action_or_reset() so the switch is cleaned up on both
probe failure and driver removal.

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index b05d80e849a14..8af5a4f856987 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -173,8 +173,10 @@ static struct typec_switch_dev *tca_blk_get_typec_switch(struct platform_device
>  	return sw;
>  }
>  
> -static void tca_blk_put_typec_switch(struct typec_switch_dev *sw)
> +static void tca_blk_put_typec_switch(void *data)
>  {
> +	struct typec_switch_dev *sw = data;
> +
>  	typec_switch_unregister(sw);
>  }

[Severity: High]
This is a pre-existing issue, but could this cleanup sequence lead to a
use-after-free during unbind?

When the PHY driver unbinds or fails probe late, the devres framework will
automatically free the imx_phy and tca structures since they are allocated
with devm_kzalloc. While typec_switch_unregister() drops the initial
reference to the switch, it doesn't force consumers to drop their references.

If a consumer typec port driver still holds a reference to the switch and
calls its .set callback after the PHY driver unbinds:

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

won't this callback access the freed imx_phy and tca memory?

[ ... ]

> @@ -266,17 +269,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 isn't a bug introduced by this patch, but does registering the switch
here expose it to consumers before imx_phy->tca is assigned?

Once registered, the switch becomes globally discoverable via fwnode matching.
If a consumer discovers it and immediately invokes the .set callback
(tca_blk_typec_switch_set), that callback expects imx_phy->tca to be
fully initialized:

tca_blk_typec_switch_set()
    struct tca_blk *tca = imx_phy->tca;
    int ret;

    if (tca->orientation == orientation)

Since the assignment of imx_phy->tca happens later in the probe sequence
after this function returns, wouldn't this concurrent access 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);
>  
> -	tca_blk_put_typec_switch(tca->sw);
> +	return tca;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-imx8mq-typec-v1-1-3debe9bc77d9@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy