[PATCH] phy: omap-usb2: add explicit PHY comparator API

[PATCH] phy: omap-usb2: add explicit PHY comparator API

[Ivaylo Dimitrov]

The existing omap_usb2_set_comparator() API locates the OMAP USB2 PHY
instance by calling usb_get_phy(USB_PHY_TYPE_USB2), assuming there is a
single USB2 PHY registered in the system.

This assumption no longer holds on systems with multiple USB2 PHY
providers. In such cases, the global lookup may return a different USB2
PHY instance, causing the OMAP driver to perform an invalid container_of()
conversion when accessing its private data.

Introduce omap_usb2_set_phy_comparator(), allowing callers to explicitly
specify the OMAP USB2 PHY instance to associate with a phy_companion.
The new helper validates that the supplied USB PHY is an OMAP USB2 PHY
before accessing its private data.

The existing omap_usb2_set_comparator() API is kept unchanged for
existing users.


Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/phy/ti/phy-omap-usb2.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/phy/omap_usb.h   |  7 +++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/phy/ti/phy-omap-usb2.c b/drivers/phy/ti/phy-omap-usb2.c
index 1eb252604441..ac31b693fce7 100644
--- a/drivers/phy/ti/phy-omap-usb2.c
+++ b/drivers/phy/ti/phy-omap-usb2.c
@@ -70,6 +70,8 @@ struct omap_usb {
 
 #define	phy_to_omapusb(x)	container_of((x), struct omap_usb, phy)
 
+static int omap_usb_set_host(struct usb_otg *otg, struct usb_bus *host);
+
 struct usb_phy_data {
 	const char *label;
 	u8 flags;
@@ -113,6 +115,36 @@ int omap_usb2_set_comparator(struct phy_companion *comparator)
 }
 EXPORT_SYMBOL_GPL(omap_usb2_set_comparator);
 
+/**
+ * omap_usb2_set_phy_comparator() - associate a comparator with an OMAP USB2 PHY
+ *
+ * @omap_phy: OMAP USB2 PHY instance
+ * @comparator: companion PHY providing comparator operations
+ *
+ * The phy companion driver should call this API with the companion PHY
+ * implementation containing the callbacks required by @omap_phy.
+ *
+ * For use by phy companion drivers.
+ */
+int omap_usb2_set_phy_comparator(struct usb_phy *omap_phy,
+				 struct phy_companion *comparator)
+{
+	struct omap_usb	*phy;
+
+	if (!omap_phy)
+		return -EINVAL;
+
+	if (WARN_ON(!omap_phy->otg ||
+		    omap_phy->otg->set_host != omap_usb_set_host))
+		return -EINVAL;
+
+	phy = phy_to_omapusb(omap_phy);
+	phy->comparator = comparator;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(omap_usb2_set_phy_comparator);
+
 static int omap_usb_set_vbus(struct usb_otg *otg, bool enabled)
 {
 	struct omap_usb *phy = phy_to_omapusb(otg->usb_phy);
diff --git a/include/linux/phy/omap_usb.h b/include/linux/phy/omap_usb.h
index e23b52df93ec..96f9792c6ce6 100644
--- a/include/linux/phy/omap_usb.h
+++ b/include/linux/phy/omap_usb.h
@@ -15,11 +15,18 @@
 
 #if defined(CONFIG_OMAP_USB2) || defined(CONFIG_OMAP_USB2_MODULE)
 extern int omap_usb2_set_comparator(struct phy_companion *comparator);
+extern int omap_usb2_set_phy_comparator(struct usb_phy *omap_phy,
+					struct phy_companion *comparator);
 #else
 static inline int omap_usb2_set_comparator(struct phy_companion *comparator)
 {
 	return -ENODEV;
 }
+static inline int omap_usb2_set_phy_comparator(struct usb_phy *omap_phy,
+					       struct phy_companion *comparator)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __DRIVERS_OMAP_USB_H */
-- 
2.39.5


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

Re: [PATCH] phy: omap-usb2: add explicit PHY comparator API

[sashiko-bot]

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

New issues:
- [Medium] Type validation relies on function pointer equality, which is unsafe under Identical Code Folding (ICF).

Pre-existing issues:
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race condition exists when accessing `phy->comparator`.
- [High] The legacy `omap_usb2_set_comparator()` function remains vulnerable to an invalid `container_of()` conversion on multi-PHY systems.
--

--- Patch [1]: [PATCH] phy: omap-usb2: add explicit PHY comparator API ---
commit 4b00e72aaba66e3ef6417d57ee7a7916dac1cd3d
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

phy: omap-usb2: add explicit PHY comparator API

The existing omap_usb2_set_comparator() API assumes a single USB2 PHY,
leading to invalid container_of() casts on multi-PHY systems. This commit
introduces omap_usb2_set_phy_comparator() for explicit PHY instance
association while keeping the legacy API intact for existing users.

> diff --git a/drivers/phy/ti/phy-omap-usb2.c b/drivers/phy/ti/phy-omap-usb2.c
> index 1eb252604441b..ac31b693fce7b 100644
> --- a/drivers/phy/ti/phy-omap-usb2.c
> +++ b/drivers/phy/ti/phy-omap-usb2.c
[ ... ]
> @@ -113,6 +115,36 @@ int omap_usb2_set_comparator(struct phy_companion *comparator)
>  }
>  EXPORT_SYMBOL_GPL(omap_usb2_set_comparator);

[Severity: High]
This is a pre-existing issue, but does leaving this legacy API without type
validation defeat the purpose of fixing the multi-PHY issue?

If an existing caller on a multi-PHY system uses this API and usb_get_phy()
returns a non-OMAP PHY, it will still perform an invalid container_of() cast:

drivers/phy/ti/phy-omap-usb2.c:omap_usb2_set_comparator() {
    ...
    struct usb_phy *x = usb_get_phy(USB_PHY_TYPE_USB2);
    ...
    phy = phy_to_omapusb(x);
    phy->comparator = comparator;
    ...
}

Could this lead to arbitrary memory corruption when phy->comparator is
overwritten on a foreign PHY structure?

> +/**
> + * omap_usb2_set_phy_comparator() - associate a comparator with an OMAP USB2 PHY
> + *
> + * @omap_phy: OMAP USB2 PHY instance
> + * @comparator: companion PHY providing comparator operations
> + *
> + * The phy companion driver should call this API with the companion PHY
> + * implementation containing the callbacks required by @omap_phy.
> + *
> + * For use by phy companion drivers.
> + */
> +int omap_usb2_set_phy_comparator(struct usb_phy *omap_phy,
> +				 struct phy_companion *comparator)
> +{
> +	struct omap_usb	*phy;
> +
> +	if (!omap_phy)
> +		return -EINVAL;
> +
> +	if (WARN_ON(!omap_phy->otg ||
> +		    omap_phy->otg->set_host != omap_usb_set_host))
> +		return -EINVAL;

[Severity: Medium]
Is it safe to rely on function pointer equality for type validation here?

If the kernel is built with Link Time Optimization and Identical Code Folding
(e.g., Clang LTO --icf=all), omap_usb_set_host could be folded into the
same memory address as a structurally identical set_host implementation
in another PHY driver.

Under these conditions, could this check falsely pass for an incorrect PHY,
leading to an invalid container_of() cast?

> +
> +	phy = phy_to_omapusb(omap_phy);
> +	phy->comparator = comparator;

[Severity: High]
This is a pre-existing issue, but does modifying the shared phy->comparator
pointer locklessly create a race condition with OTG events?

Looking at omap_usb_set_vbus() and omap_usb_start_srp(), the comparator
pointer is checked and then dereferenced without locks or READ_ONCE():

drivers/phy/ti/phy-omap-usb2.c:omap_usb_set_vbus() {
    ...
    if (!phy->comparator || !phy->comparator->set_vbus)
        return -ENODEV;

    return phy->comparator->set_vbus(phy->comparator, enabled);
}

If an OTG event occurs concurrently with a device unbind that clears the
comparator pointer, could this result in a use-after-free or NULL pointer
dereference?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(omap_usb2_set_phy_comparator);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722053618.602702-1-ivo.g.dimitrov.75@gmail.com?part=1

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