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

[sashiko-bot@kernel.org]

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