Re: [PATCH v4 1/5] 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] Race condition during probe leading to NULL pointer dereference.
- [High] Use-After-Free (UAF) of provider driver data on manual unbind.
--

commit 9dde5dedb4fbdef1b93e7e96ccdb22fc9407f2cc
Author: Felix Gu <ustc.gu@gmail.com>

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

This commit fixes a memory leak of the Type-C switch on the probe error
path by using devm_add_action_or_reset() for cleanup. This ensures the
switch is unregistered 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..88b804b2c982b 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c

[ ... ]

> @@ -248,6 +248,7 @@ static struct tca_blk *imx95_usb_phy_get_tca(struct platform_device *pdev,
>  	struct device *dev = &pdev->dev;
>  	struct resource *res;
>  	struct tca_blk *tca;
> +	int ret;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>  	if (!res)
> @@ -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
during probe?

When tca_blk_get_typec_switch() registers the Type-C switch, it becomes
exposed to the subsystem. If a consumer binds asynchronously in this narrow
window and invokes the .set callback, could it dereference imx_phy->tca
before it gets assigned back in imx8mq_usb_phy_probe()?

drivers/phy/freescale/phy-fsl-imx8mq-usb.c:tca_blk_typec_switch_set() {
	struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
	struct tca_blk *tca = imx_phy->tca;
	...
	if (tca->orientation == orientation)
		...
}

Would it be safer to assign imx_phy->tca before registering the
Type-C switch?

> -	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);

[Severity: High]
This is also a pre-existing issue, but can this lead to a Use-After-Free
if the driver is manually unbound?

If the driver is unbound via sysfs, devres will automatically free the
imx_phy and tca structures since they were allocated with devm_kzalloc().
However, if typec_switch_unregister() doesn't revoke existing consumer
references, could a consumer subsequently call typec_switch_set() and
access the already freed imx_phy and tca structures?

drivers/phy/freescale/phy-fsl-imx8mq-usb.c:tca_blk_typec_switch_set() {
	struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
	struct tca_blk *tca = imx_phy->tca;
	...
	if (tca->orientation == orientation)
		...
}

> +	if (ret)
> +		return ERR_PTR(ret);
>  
> -	tca_blk_put_typec_switch(tca->sw);
> +	return tca;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-imx8mp-usb-phy-improvement-v4-0-b2ddf2f3862c@nxp.com?part=1

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