Re: [PATCH v3 08/13] usb: hub: Power on connected M.2 E-key connectors with power sequencing API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Off-by-one array access in `hub_is_port_power_switchable()` causes an out-of-bounds read and potential use-after-free on the `hub->ports` array.
- [High] Missing NULL check for `port->pwrseq` causes all USB ports to fail to power on when `CONFIG_POWER_SEQUENCING` is disabled.
- [Medium] Leaked firmware node reference count in `usb_hub_create_port_device`.
- [Low] Missing dummy declaration for `pwrseq_power_is_on()` when `CONFIG_POWER_SEQUENCING` is disabled.
--

commit d1a665b6f212fa1e434ea2cca8c69acf5a200a73
Author: Chen-Yu Tsai <wenst@chromium.org>

    usb: hub: Power on connected M.2 E-key connectors with power sequencing API
    
    The new M.2 E-key connector can have a USB connection. For the USB device
    on this connector to work, its power must be enabled and the W_DISABLE2#
    signal deasserted. The connector driver handles this and provides a
    toggle over the power sequencing API.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 5969c55b2201f..5327ae179181d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
[ ... ]
> @@ -871,6 +872,30 @@ static void hub_tt_work(struct work_struct *work)
>  	spin_unlock_irqrestore(&hub->tt.lock, flags);
>  }
>  
> +static int usb_hub_set_port_pwrseq(struct usb_port *port, bool set)
> +{
> +	int ret = 0;
> +
> +	if (set)
> +		ret = pwrseq_power_on(port->pwrseq);
> +	else
> +		ret = pwrseq_power_off(port->pwrseq);
> +
> +	return ret;
> +}

[Severity: High]
Does this code correctly handle the case when CONFIG_POWER_SEQUENCING is
disabled?

When power sequencing is disabled, port->pwrseq is NULL. However, the
inline stub for pwrseq_power_on() in include/linux/pwrseq/consumer.h blindly
returns -ENOSYS.

Because this return value is checked in usb_hub_set_port_power():

drivers/usb/core/hub.c:usb_hub_set_port_power() {
    ...
    ret = usb_hub_set_port_pwrseq(pwrseq_port, set);
    if (ret)
        return ret;
    ...
}

Could this cause all USB ports to completely fail to power on when the kernel
is built without CONFIG_POWER_SEQUENCING support, returning -ENOSYS before
calling set_port_feature()?

[ ... ]
> @@ -3242,7 +3271,10 @@ int usb_port_is_power_on(struct usb_port *port, unsigned int portstatus)
>  			ret = 1;
>  	}
>  
> -	return ret;
> +	if (!port->pwrseq)
> +		return ret;
> +
> +	return ret && pwrseq_power_is_on(port->pwrseq);
>  }

[Severity: Low]
Is there a missing dummy declaration for pwrseq_power_is_on() when
CONFIG_POWER_SEQUENCING is disabled?

It appears that the fallback #else block in include/linux/pwrseq/consumer.h
is missing a dummy stub implementation for pwrseq_power_is_on(). This could
lead to an implicit function declaration error when building the kernel with
CONFIG_USB=y and CONFIG_POWER_SEQUENCING=n.

> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index b65d9192379db..99bae6ace4da0 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
[ ... ]
> @@ -147,7 +149,13 @@ static inline bool hub_is_port_power_switchable(struct usb_hub *hub)
>  	if (!hub)
>  		return false;
>  	hcs = hub->descriptor->wHubCharacteristics;
> -	return (le16_to_cpu(hcs) & HUB_CHAR_LPSM) < HUB_CHAR_NO_LPSM;
> +	if ((le16_to_cpu(hcs) & HUB_CHAR_LPSM) < HUB_CHAR_NO_LPSM)
> +		return true;
> +	/* check for controllable external power sequencers */
> +	for (unsigned int i = 1; i <= hub->hdev->maxchild; i++)
> +		if (hub->ports[i] && hub->ports[i]->pwrseq)
> +			return true;
> +	return false;
>  }

[Severity: High]
Does this loop index cause an out-of-bounds read and potential use-after-free
on the hub->ports array?

The loop iterates from i = 1 to hub->hdev->maxchild, skipping index 0 and
reading one element past the allocated boundary (index maxchild). 

Additionally, if usb_hub_create_port_device() fails to initialize a port,
the port device pointer is still assigned in the array:

drivers/usb/core/port.c:usb_hub_create_port_device() {
    ...
    hub->ports[port1 - 1] = port_dev;
    ...
    retval = device_register(&port_dev->dev);
    if (retval) {
        put_device(&port_dev->dev);
        return retval;
    }
    ...
}

If this initialization fails for port X, hub->ports[X-1] will retain a pointer
to the freed port device. When hub_is_port_power_switchable() iterates,
it will access hub->ports[1] through hub->ports[X-1], leading to a
use-after-free when reading the ->pwrseq field.

> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 77dbf51f17604..87b8e4f90be68 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
[ ... ]
> @@ -809,10 +848,18 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  		goto err_put_kn;
>  	}
>  
> +	port_dev->pwrseq = usb_hub_port_pwrseq_get(port_dev);
> +	if (IS_ERR(port_dev->pwrseq)) {
> +		retval = PTR_ERR(port_dev->pwrseq);
> +		dev_err_probe(&port_dev->dev, retval,
> +			      "failed to get power sequencing descriptor\n");
> +		goto err_put_kn;
> +	}
> +

[Severity: Medium]
Can this code leak a firmware node reference count?

Earlier in usb_hub_create_port_device(), fwnode_graph_get_port_by_id()
returns a fwnode_handle with its reference count incremented:

drivers/usb/core/port.c:usb_hub_create_port_device() {
    ...
    if (!is_acpi_node(dev_fwnode(&hdev->dev)))
        device_set_node(&port_dev->dev,
                fwnode_graph_get_port_by_id(dev_fwnode(&hdev->dev), port1));
    ...
}

This fwnode_handle is assigned to the device, but the device core does not
automatically drop it on device_unregister(). Since fwnode_handle_put() is
never called in usb_port_device_release() or usb_hub_remove_port_device(),
is this reference permanently leaked during port device teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703110317.1283411-1-wenst@chromium.org?part=8