| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <4fe5b63e-072c-419c-a1b9-bc21aec7e083@app.fastmail.com>
Date: Mon, 17 Nov 2025 09:53:21 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Ma Ke" <make24@...as.ac.cn>, "Alan Stern" <stern@...land.harvard.edu>,
"Vladimir Zapolskiy" <vz@...ia.com>, piotr.wojtaszczyk@...esys.com,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>, stigge@...com.de
Cc: linux-usb@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, "Andrew Morton" <akpm@...ux-foundation.org>,
stable@...r.kernel.org
Subject: Re: [PATCH v2] USB: ohci-nxp: Fix error handling in ohci-hcd-nxp driver
On Mon, Nov 17, 2025, at 02:34, Ma Ke wrote:
> When obtaining the ISP1301 I2C client through the device tree, the
> driver does not release the device reference in the probe failure path
> or in the remove function. This could cause a reference count leak,
> which may prevent the device from being properly unbound or freed,
> leading to resource leakage.
>
> Fix this by storing whether the client was obtained via device tree
> and only releasing the reference in that case.
>
> Found by code review.
>
> Cc: stable@...r.kernel.org
> Fixes: 73108aa90cbf ("USB: ohci-nxp: Use isp1301 driver")
> Signed-off-by: Ma Ke <make24@...as.ac.cn>
The patch looks fine in principle, however I don't see any way
this driver would be probed without devicetree, and I think
it would be better to remove all the traces of the pre-DT
logic in it.
The lpc32xx platform was converted to DT back in 2012, so
any reference to the old variant is dead code. Something like
the patch below should work here.
Other thoughts on this driver, though I I'm not sure anyone
is going to have the energy to implement these:
- the reference to isp1301_i2c_client should be kept in
the hcd private data, after allocating a structure, by
setting driver->hcd_priv_size.
- instead of looking for the i2c device, I would suppose
it should look for a usb_phy instead, as there is no
guarantee on the initialization being ordered at the
moment.
- instead of a usb_phy, the driver should probably use
a generic phy (a much larger rework).
Arnd
diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
index 24d5a1dc5056..4c072ce02f4d 100644
--- a/drivers/usb/host/ohci-nxp.c
+++ b/drivers/usb/host/ohci-nxp.c
@@ -155,22 +155,12 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
struct device_node *isp1301_node;
struct clk *usb_host_clk;
- if (pdev->dev.of_node) {
- isp1301_node = of_parse_phandle(pdev->dev.of_node,
- "transceiver", 0);
- } else {
- isp1301_node = NULL;
- }
-
- isp1301_i2c_client = isp1301_get_client(isp1301_node);
+ isp1301_node = of_parse_phandle(pdev->dev.of_node, "transceiver", 0);
+ isp1301_i2c_client = of_find_i2c_device_by_node(isp1301_node);
of_node_put(isp1301_node);
if (!isp1301_i2c_client)
return -EPROBE_DEFER;
- ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
- if (ret)
- goto fail_disable;
-
dev_dbg(&pdev->dev, "%s: " DRIVER_DESC " (nxp)\n", hcd_name);
if (usb_disabled()) {
dev_err(&pdev->dev, "USB is disabled\n");
@@ -223,7 +213,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
fail_resource:
usb_put_hcd(hcd);
fail_disable:
- isp1301_i2c_client = NULL;
+ put_device(isp1301_i2c_client);
return ret;
}
@@ -234,24 +224,19 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
usb_remove_hcd(hcd);
ohci_nxp_stop_hc();
usb_put_hcd(hcd);
- isp1301_i2c_client = NULL;
+ put_device(isp1301_i2c_client);
}
-/* work with hotplug and coldplug */
-MODULE_ALIAS("platform:usb-ohci");
-
-#ifdef CONFIG_OF
static const struct of_device_id ohci_hcd_nxp_match[] = {
{ .compatible = "nxp,ohci-nxp" },
{},
};
MODULE_DEVICE_TABLE(of, ohci_hcd_nxp_match);
-#endif
static struct platform_driver ohci_hcd_nxp_driver = {
.driver = {
- .name = "usb-ohci",
- .of_match_table = of_match_ptr(ohci_hcd_nxp_match),
+ .name = "usb-ohci-lpc32xx",
+ .of_match_table = ohci_hcd_nxp_match,
},
.probe = ohci_hcd_nxp_probe,
.remove = ohci_hcd_nxp_remove,
diff --git a/drivers/usb/phy/phy-isp1301.c b/drivers/usb/phy/phy-isp1301.c
index f9b5c411aee4..3a8fa333a4f7 100644
--- a/drivers/usb/phy/phy-isp1301.c
+++ b/drivers/usb/phy/phy-isp1301.c
@@ -24,20 +24,12 @@ struct isp1301 {
#define phy_to_isp(p) (container_of((p), struct isp1301, phy))
-static const struct i2c_device_id isp1301_id[] = {
- { "isp1301" },
- { }
-};
-MODULE_DEVICE_TABLE(i2c, isp1301_id);
-
static const struct of_device_id isp1301_of_match[] = {
{.compatible = "nxp,isp1301" },
{ },
};
MODULE_DEVICE_TABLE(of, isp1301_of_match);
-static struct i2c_client *isp1301_i2c_client;
-
static int __isp1301_write(struct isp1301 *isp, u8 reg, u8 value, u8 clear)
{
return i2c_smbus_write_byte_data(isp->client, reg | clear, value);
@@ -114,8 +106,6 @@ static int isp1301_probe(struct i2c_client *client)
i2c_set_clientdata(client, isp);
usb_add_phy_dev(phy);
- isp1301_i2c_client = client;
-
return 0;
}
@@ -124,7 +114,6 @@ static void isp1301_remove(struct i2c_client *client)
struct isp1301 *isp = i2c_get_clientdata(client);
usb_remove_phy(&isp->phy);
- isp1301_i2c_client = NULL;
}
static struct i2c_driver isp1301_driver = {
@@ -134,25 +123,10 @@ static struct i2c_driver isp1301_driver = {
},
.probe = isp1301_probe,
.remove = isp1301_remove,
- .id_table = isp1301_id,
};
module_i2c_driver(isp1301_driver);
-struct i2c_client *isp1301_get_client(struct device_node *node)
-{
- struct i2c_client *client;
-
- /* reference of ISP1301 I2C node via DT */
- client = of_find_i2c_device_by_node(node);
- if (client)
- return client;
-
- /* non-DT: only one ISP1301 chip supported */
- return isp1301_i2c_client;
-}
-EXPORT_SYMBOL_GPL(isp1301_get_client);
-
MODULE_AUTHOR("Roland Stigge <stigge@...com.de>");
MODULE_DESCRIPTION("NXP ISP1301 USB transceiver driver");
MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/isp1301.h b/include/linux/usb/isp1301.h
index fa986b926a12..135dd785d984 100644
--- a/include/linux/usb/isp1301.h
+++ b/include/linux/usb/isp1301.h
@@ -66,6 +66,4 @@
#define ISP1301_I2C_REG_CLEAR_ADDR 1 /* Register Address Modifier */
-struct i2c_client *isp1301_get_client(struct device_node *node);
-
#endif /* __LINUX_USB_ISP1301_H */
Powered by blists - more mailing lists