Re: [PATCH v5 10/16] 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 2 potential issue(s) to consider:
- [High] Concurrent sysfs `disable` read or write can dereference an `ERR_PTR` if `usb_hub_port_pwrseq_get()` fails.
- [High] usb_hub_set_port_power() accesses the dynamically allocated hub->ports array, which can be freed concurrently by hub_disconnect().
--

--- Patch [10]: [PATCH v5 10/16] usb: hub: Power on connected M.2 E-key connectors with power sequencing API ---
commit e0346a1be7fb5d1254f3498bb6d857dd5db5a9d7
Author: Chen-Yu Tsai <wenst@chromium.org>
Subject: usb: hub: Power on connected M.2 E-key connectors with power sequencing API

This patch integrates the power sequencing API with USB ports to support M.2
E-key connectors. It manages power sequencing during port creation, removal,
and power state changes.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 10f15b40fcc55..298fb68c6afc7 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

[ ... ]

> @@ -887,15 +898,22 @@ int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub,
>  			   int port1, bool set)
>  {
> +	struct usb_port *pwrseq_port = hub->ports[port1 - 1];

[Severity: High]
Can this code trigger a use-after-free on the hub->ports array?

The sysfs disable_store callback uses sysfs_break_active_protection()
to avoid deadlocks, meaning it prevents device_unregister() from waiting
for the callback to finish.

If hub_probe() fails and triggers hub_disconnect() concurrently,
hub_disconnect() will call device_unregister() which returns immediately.
Then hub_disconnect() unconditionally frees the hub->ports array:

hub_disconnect() {
    ...
    kfree(hub->ports);
}

Meanwhile, disable_store() can continue and call usb_hub_set_port_power(),
which now accesses the freed hub->ports array. Prior to this change,
usb_hub_set_port_power() only accessed inline fields of the refcounted
hub struct.

Is there a way to safely access the port here without racing against the array
being freed?

> +	int ret;
>  
> +	ret = usb_hub_set_port_pwrseq(pwrseq_port, set);
> +	if (ret)
> +		return ret;

[ ... ]

> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 19913ad3598b3..430fc90232502 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c

[ ... ]

> @@ -831,6 +879,13 @@ 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 = dev_err_probe(&port_dev->dev, PTR_ERR(port_dev->pwrseq),
> +				       "failed to get power sequencing descriptor\n");
> +		goto err_put_kn;
> +	}

[Severity: High]
Could this temporarily expose an ERR_PTR to concurrent sysfs operations?

The port device is registered earlier in this function via device_register(),
exposing its sysfs attributes (like disable) before usb_hub_port_pwrseq_get()
is called.

If acquiring the power sequencer fails and returns an ERR_PTR,
port_dev->pwrseq will hold this ERR_PTR. Because sysfs unregistration waits
for active callbacks on the error path, a concurrent read or write to the
disable attribute could access port_dev->pwrseq while it holds an error value.

This ERR_PTR would then be passed to usb_port_is_power_on() or
usb_hub_set_port_pwrseq(), which call the power sequencer APIs:

static int usb_hub_set_port_pwrseq(struct usb_port *port, bool on)
{
    if (!IS_ENABLED(CONFIG_POWER_SEQUENCING))
        return 0;
        
    if (on)
        return pwrseq_power_on(port->pwrseq);
    return pwrseq_power_off(port->pwrseq);
}

Since the pwrseq_power_* APIs do not check for ERR_PTR, would this cause
a panic when dereferencing the pointer?

> +
>  	retval = component_add(&port_dev->dev, &connector_ops);
>  	if (retval) {
>  		dev_warn(&port_dev->dev, "failed to add component\n");

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