[PATCH v2 1/5] usb: gadget: lpc32xx_udc: fix clock imbalance in error path

[PATCH v2 1/5] usb: gadget: lpc32xx_udc: fix clock imbalance in error path

[Johan Hovold]

A recent change fixing a device reference leak introduced a clock
imbalance by reusing an error path so that the clock may be disabled
before having been enabled.

Note that the clock framework allows for passing in NULL clocks so there
is no risk for a NULL pointer dereference.

Also drop the bogus I2C client NULL check added by the offending commit
as the pointer has already been verified to be non-NULL.

Fixes: c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
Cc: stable@vger.kernel.org
Cc: Ma Ke <make24@iscas.ac.cn>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/gadget/udc/lpc32xx_udc.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
index 73c0f28a8585..a962d4294fbe 100644
--- a/drivers/usb/gadget/udc/lpc32xx_udc.c
+++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
@@ -3020,7 +3020,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	pdev->dev.dma_mask = &lpc32xx_usbd_dmamask;
 	retval = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 	if (retval)
-		goto i2c_fail;
+		goto err_put_client;
 
 	udc->board = &lpc32xx_usbddata;
 
@@ -3040,7 +3040,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 		udc->udp_irq[i] = platform_get_irq(pdev, i);
 		if (udc->udp_irq[i] < 0) {
 			retval = udc->udp_irq[i];
-			goto i2c_fail;
+			goto err_put_client;
 		}
 	}
 
@@ -3048,7 +3048,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (IS_ERR(udc->udp_baseaddr)) {
 		dev_err(udc->dev, "IO map failure\n");
 		retval = PTR_ERR(udc->udp_baseaddr);
-		goto i2c_fail;
+		goto err_put_client;
 	}
 
 	/* Get USB device clock */
@@ -3056,14 +3056,14 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (IS_ERR(udc->usb_slv_clk)) {
 		dev_err(udc->dev, "failed to acquire USB device clock\n");
 		retval = PTR_ERR(udc->usb_slv_clk);
-		goto i2c_fail;
+		goto err_put_client;
 	}
 
 	/* Enable USB device clock */
 	retval = clk_prepare_enable(udc->usb_slv_clk);
 	if (retval < 0) {
 		dev_err(udc->dev, "failed to start USB device clock\n");
-		goto i2c_fail;
+		goto err_put_client;
 	}
 
 	/* Setup deferred workqueue data */
@@ -3165,9 +3165,10 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
 			  udc->udca_v_base, udc->udca_p_base);
 i2c_fail:
-	if (udc->isp1301_i2c_client)
-		put_device(&udc->isp1301_i2c_client->dev);
 	clk_disable_unprepare(udc->usb_slv_clk);
+err_put_client:
+	put_device(&udc->isp1301_i2c_client->dev);
+
 	dev_err(udc->dev, "%s probe failed, %d\n", driver_name, retval);
 
 	return retval;
@@ -3195,10 +3196,9 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
 	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
 			  udc->udca_v_base, udc->udca_p_base);
 
-	if (udc->isp1301_i2c_client)
-		put_device(&udc->isp1301_i2c_client->dev);
-
 	clk_disable_unprepare(udc->usb_slv_clk);
+
+	put_device(&udc->isp1301_i2c_client->dev);
 }
 
 #ifdef CONFIG_PM
-- 
2.51.2

Re: [PATCH v2 1/5] usb: gadget: lpc32xx_udc: fix clock imbalance in error path

[Vladimir Zapolskiy]

Hi Johan,

thank you for looking at the issue.

On 12/18/25 17:35, Johan Hovold wrote:
> A recent change fixing a device reference leak introduced a clock
> imbalance by reusing an error path so that the clock may be disabled
> before having been enabled.
> 
> Note that the clock framework allows for passing in NULL clocks so there
> is no risk for a NULL pointer dereference.
> 
> Also drop the bogus I2C client NULL check added by the offending commit
> as the pointer has already been verified to be non-NULL.
> 
> Fixes: c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")

This commit didn't get a proper review, and it is broken in so many ways,
that I'd say it has to be reverted.

> Cc: stable@vger.kernel.org
> Cc: Ma Ke <make24@iscas.ac.cn>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/usb/gadget/udc/lpc32xx_udc.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
> index 73c0f28a8585..a962d4294fbe 100644
> --- a/drivers/usb/gadget/udc/lpc32xx_udc.c
> +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
> @@ -3020,7 +3020,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
>   	pdev->dev.dma_mask = &lpc32xx_usbd_dmamask;
>   	retval = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
>   	if (retval)
> -		goto i2c_fail;
> +		goto err_put_client;
>   
>   	udc->board = &lpc32xx_usbddata;
>   
> @@ -3040,7 +3040,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
>   		udc->udp_irq[i] = platform_get_irq(pdev, i);
>   		if (udc->udp_irq[i] < 0) {
>   			retval = udc->udp_irq[i];
> -			goto i2c_fail;
> +			goto err_put_client;
>   		}
>   	}
>   
> @@ -3048,7 +3048,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
>   	if (IS_ERR(udc->udp_baseaddr)) {
>   		dev_err(udc->dev, "IO map failure\n");
>   		retval = PTR_ERR(udc->udp_baseaddr);
> -		goto i2c_fail;
> +		goto err_put_client;
>   	}
>   
>   	/* Get USB device clock */
> @@ -3056,14 +3056,14 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
>   	if (IS_ERR(udc->usb_slv_clk)) {
>   		dev_err(udc->dev, "failed to acquire USB device clock\n");
>   		retval = PTR_ERR(udc->usb_slv_clk);
> -		goto i2c_fail;
> +		goto err_put_client;
>   	}
>   
>   	/* Enable USB device clock */
>   	retval = clk_prepare_enable(udc->usb_slv_clk);
>   	if (retval < 0) {
>   		dev_err(udc->dev, "failed to start USB device clock\n");
> -		goto i2c_fail;
> +		goto err_put_client;
>   	}
>   
>   	/* Setup deferred workqueue data */
> @@ -3165,9 +3165,10 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
>   	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
>   			  udc->udca_v_base, udc->udca_p_base);
>   i2c_fail:
> -	if (udc->isp1301_i2c_client)
> -		put_device(&udc->isp1301_i2c_client->dev);
>   	clk_disable_unprepare(udc->usb_slv_clk);
> +err_put_client:
> +	put_device(&udc->isp1301_i2c_client->dev);
> +
>   	dev_err(udc->dev, "%s probe failed, %d\n", driver_name, retval);
>   
>   	return retval;
> @@ -3195,10 +3196,9 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
>   	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
>   			  udc->udca_v_base, udc->udca_p_base);
>   
> -	if (udc->isp1301_i2c_client)
> -		put_device(&udc->isp1301_i2c_client->dev);
> -
>   	clk_disable_unprepare(udc->usb_slv_clk);
> +
> +	put_device(&udc->isp1301_i2c_client->dev);
>   }
>   
>   #ifdef CONFIG_PM

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

-- 
Best wishes,
Vladimir

[PATCH v2 2/5] usb: phy: isp1301: fix non-OF device reference imbalance

[Johan Hovold]

A recent change fixing a device reference leak in a UDC driver
introduced a potential use-after-free in the non-OF case as the
isp1301_get_client() helper only increases the reference count for the
returned I2C device in the OF case.

Increment the reference count also for non-OF so that the caller can
decrement it unconditionally.

Note that this is inherently racy just as using the returned I2C device
is since nothing is preventing the PHY driver from being unbound while
in use.

Fixes: c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
Cc: stable@vger.kernel.org
Cc: Ma Ke <make24@iscas.ac.cn>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/phy/phy-isp1301.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-isp1301.c b/drivers/usb/phy/phy-isp1301.c
index f9b5c411aee4..2940f0c84e1b 100644
--- a/drivers/usb/phy/phy-isp1301.c
+++ b/drivers/usb/phy/phy-isp1301.c
@@ -149,7 +149,12 @@ struct i2c_client *isp1301_get_client(struct device_node *node)
 		return client;
 
 	/* non-DT: only one ISP1301 chip supported */
-	return isp1301_i2c_client;
+	if (isp1301_i2c_client) {
+		get_device(&isp1301_i2c_client->dev);
+		return isp1301_i2c_client;
+	}
+
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(isp1301_get_client);
 
-- 
2.51.2

Re: [PATCH v2 2/5] usb: phy: isp1301: fix non-OF device reference imbalance

[Vladimir Zapolskiy]

On 12/18/25 17:35, Johan Hovold wrote:
> A recent change fixing a device reference leak in a UDC driver
> introduced a potential use-after-free in the non-OF case as the
> isp1301_get_client() helper only increases the reference count for the
> returned I2C device in the OF case.

Fortunatly there is no non-OF users of this driver, it's been discussed
recently.

> 
> Increment the reference count also for non-OF so that the caller can
> decrement it unconditionally.
> 
> Note that this is inherently racy just as using the returned I2C device
> is since nothing is preventing the PHY driver from being unbound while
> in use.
> 
> Fixes: c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
> Cc: stable@vger.kernel.org
> Cc: Ma Ke <make24@iscas.ac.cn>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/usb/phy/phy-isp1301.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/phy/phy-isp1301.c b/drivers/usb/phy/phy-isp1301.c
> index f9b5c411aee4..2940f0c84e1b 100644
> --- a/drivers/usb/phy/phy-isp1301.c
> +++ b/drivers/usb/phy/phy-isp1301.c
> @@ -149,7 +149,12 @@ struct i2c_client *isp1301_get_client(struct device_node *node)
>   		return client;
>   
>   	/* non-DT: only one ISP1301 chip supported */
> -	return isp1301_i2c_client;
> +	if (isp1301_i2c_client) {
> +		get_device(&isp1301_i2c_client->dev);
> +		return isp1301_i2c_client;
> +	}
> +
> +	return NULL;
>   }
>   EXPORT_SYMBOL_GPL(isp1301_get_client);
>   

Okay, let's go the way of fixing the broken commit instead of its reversal.

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

-- 
Best wishes,
Vladimir

Re: [PATCH v2 2/5] usb: phy: isp1301: fix non-OF device reference imbalance

[Johan Hovold]

On Fri, Dec 19, 2025 at 02:15:12AM +0200, Vladimir Zapolskiy wrote:
> On 12/18/25 17:35, Johan Hovold wrote:
> > A recent change fixing a device reference leak in a UDC driver
> > introduced a potential use-after-free in the non-OF case as the
> > isp1301_get_client() helper only increases the reference count for the
> > returned I2C device in the OF case.
> 
> Fortunatly there is no non-OF users of this driver, it's been discussed
> recently.

Yeah, I saw the discussion, but figured it was best to just fix up the
existing code before you guys get on with ripping out the legacy
support.

> > Increment the reference count also for non-OF so that the caller can
> > decrement it unconditionally.
> > 
> > Note that this is inherently racy just as using the returned I2C device
> > is since nothing is preventing the PHY driver from being unbound while
> > in use.
> > 
> > Fixes: c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
> > Cc: stable@vger.kernel.org
> > Cc: Ma Ke <make24@iscas.ac.cn>
> > Signed-off-by: Johan Hovold <johan@kernel.org>

> > @@ -149,7 +149,12 @@ struct i2c_client *isp1301_get_client(struct device_node *node)
> >   		return client;
> >   
> >   	/* non-DT: only one ISP1301 chip supported */
> > -	return isp1301_i2c_client;
> > +	if (isp1301_i2c_client) {
> > +		get_device(&isp1301_i2c_client->dev);
> > +		return isp1301_i2c_client;
> > +	}
> > +
> > +	return NULL;
> >   }
> >   EXPORT_SYMBOL_GPL(isp1301_get_client);
> >   
> 
> Okay, let's go the way of fixing the broken commit instead of its reversal.
> 
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

Thanks for reviewing.

Johan

[PATCH v2 3/5] usb: ohci-nxp: fix device leak on probe failure

[Johan Hovold]

Make sure to drop the reference taken when looking up the PHY I2C device
during probe on probe failure (e.g. probe deferral) and on driver
unbind.

Fixes: 73108aa90cbf ("USB: ohci-nxp: Use isp1301 driver")
Cc: stable@vger.kernel.org	# 3.5
Reported-by: Ma Ke <make24@iscas.ac.cn>
Link: https://lore.kernel.org/lkml/20251117013428.21840-1-make24@iscas.ac.cn/
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/host/ohci-nxp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
index 24d5a1dc5056..509ca7d8d513 100644
--- a/drivers/usb/host/ohci-nxp.c
+++ b/drivers/usb/host/ohci-nxp.c
@@ -223,6 +223,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 fail_resource:
 	usb_put_hcd(hcd);
 fail_disable:
+	put_device(&isp1301_i2c_client->dev);
 	isp1301_i2c_client = NULL;
 	return ret;
 }
@@ -234,6 +235,7 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
 	usb_remove_hcd(hcd);
 	ohci_nxp_stop_hc();
 	usb_put_hcd(hcd);
+	put_device(&isp1301_i2c_client->dev);
 	isp1301_i2c_client = NULL;
 }
 
-- 
2.51.2

Re: [PATCH v2 3/5] usb: ohci-nxp: fix device leak on probe failure

[Alan Stern]

On Thu, Dec 18, 2025 at 04:35:17PM +0100, Johan Hovold wrote:
> Make sure to drop the reference taken when looking up the PHY I2C device
> during probe on probe failure (e.g. probe deferral) and on driver
> unbind.
> 
> Fixes: 73108aa90cbf ("USB: ohci-nxp: Use isp1301 driver")
> Cc: stable@vger.kernel.org	# 3.5
> Reported-by: Ma Ke <make24@iscas.ac.cn>
> Link: https://lore.kernel.org/lkml/20251117013428.21840-1-make24@iscas.ac.cn/
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

For this patch and the 5/5 patch:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

>  drivers/usb/host/ohci-nxp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
> index 24d5a1dc5056..509ca7d8d513 100644
> --- a/drivers/usb/host/ohci-nxp.c
> +++ b/drivers/usb/host/ohci-nxp.c
> @@ -223,6 +223,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
>  fail_resource:
>  	usb_put_hcd(hcd);
>  fail_disable:
> +	put_device(&isp1301_i2c_client->dev);
>  	isp1301_i2c_client = NULL;
>  	return ret;
>  }
> @@ -234,6 +235,7 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
>  	usb_remove_hcd(hcd);
>  	ohci_nxp_stop_hc();
>  	usb_put_hcd(hcd);
> +	put_device(&isp1301_i2c_client->dev);
>  	isp1301_i2c_client = NULL;
>  }
>  
> -- 
> 2.51.2
> 

Re: [PATCH v2 3/5] usb: ohci-nxp: fix device leak on probe failure

[Vladimir Zapolskiy]

On 12/18/25 17:35, Johan Hovold wrote:
> Make sure to drop the reference taken when looking up the PHY I2C device
> during probe on probe failure (e.g. probe deferral) and on driver
> unbind.
> 
> Fixes: 73108aa90cbf ("USB: ohci-nxp: Use isp1301 driver")
> Cc: stable@vger.kernel.org	# 3.5
> Reported-by: Ma Ke <make24@iscas.ac.cn>
> Link: https://lore.kernel.org/lkml/20251117013428.21840-1-make24@iscas.ac.cn/
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/usb/host/ohci-nxp.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
> index 24d5a1dc5056..509ca7d8d513 100644
> --- a/drivers/usb/host/ohci-nxp.c
> +++ b/drivers/usb/host/ohci-nxp.c
> @@ -223,6 +223,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
>   fail_resource:
>   	usb_put_hcd(hcd);
>   fail_disable:
> +	put_device(&isp1301_i2c_client->dev);
>   	isp1301_i2c_client = NULL;
>   	return ret;
>   }
> @@ -234,6 +235,7 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
>   	usb_remove_hcd(hcd);
>   	ohci_nxp_stop_hc();
>   	usb_put_hcd(hcd);
> +	put_device(&isp1301_i2c_client->dev);
>   	isp1301_i2c_client = NULL;
>   }
>   

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

-- 
Best wishes,
Vladimir