Re: [PATCH v8 14/16] power: sequencing: pcie-m2: Split Bluetooth unit based on interface

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Splitting the Bluetooth unit into separate UART and USB units introduces a hardware state conflict where one interface can unexpectedly physically power off the device while the other is still using it.

Pre-existing issues:
- [High] Teardown asymmetry and lack of synchronization in the provider driver allows active consumers to trigger a UAF and kernel oops.
--

commit 04bd68356c9e71e9f7432f78d28c9d647b96c081
Author: Chen-Yu Tsai <wenst@chromium.org>
Subject: power: sequencing: pcie-m2: Split Bluetooth unit based on interface

This commit splits the Bluetooth power sequencing unit for M.2 PCIe connectors
into separate UART and USB units. This allows either interface to independently
toggle power, specifically enabling hardware resets during initialization or
recovery for serdev-based drivers.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index 987faa9f3b3ba..f7bcc8bb4c775 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
[ ... ]
> @@ -83,8 +83,22 @@ static int pwrseq_pci_m2_e_bt_disable(struct pwrseq_device *pwrseq)
>  	return gpiod_set_value_cansleep(ctx->w_disable2_gpio, 1);
>  }
>  
> -static const struct pwrseq_unit_data pwrseq_pcie_m2_e_bt_unit_data = {
> -	.name = "bt-enable",
> +/*
> + * XXX There are two Bluetooth units to allow either one to be able to power
> + * off and thus reset the controller. In practice only one of the interfaces
> + * is used, so there is no conflict. However userspace could power off the
> + * USB unit by disabling the associated USB port, without the UART unit or
> + * its consumer ever knowing.
> + */
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_e_bt_uart_unit_data = {
> +	.name = "bt-uart-enable",
> +	.deps = pwrseq_pcie_m2_unit_deps,
> +	.enable = pwrseq_pci_m2_e_bt_enable,
> +	.disable = pwrseq_pci_m2_e_bt_disable,
> +};
> +
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_e_bt_usb_unit_data = {
> +	.name = "bt-usb-enable",
> +	.deps = pwrseq_pcie_m2_unit_deps,
> +	.enable = pwrseq_pci_m2_e_bt_enable,
> +	.disable = pwrseq_pci_m2_e_bt_disable,
> +};

[Severity: Medium]
Since both pwrseq_pcie_m2_e_bt_uart_unit_data and 
pwrseq_pcie_m2_e_bt_usb_unit_data use the same w_disable2_gpio in their
disable callback, does this split introduce a hardware state conflict?

If userspace or the USB core disables the USB interface, the USB unit's
refcount will drop to 0 and physically assert the shared GPIO. Can this 
unexpectedly cut power to the Bluetooth controller while the UART consumer 
is still actively using it, violating its state machine?

[ ... ]

drivers/power/sequencing/pwrseq-pcie-m2.c:pwrseq_pcie_m2_remove() {
    ...
	pwrseq_pcie_m2_remove_serdev(ctx, NULL);
	mutex_destroy(&ctx->list_lock);

	regulator_bulk_free(ctx->num_vregs, ctx->regs);
}

[Severity: High]
This isn't a bug introduced by this patch, but could manually freeing the 
regulators here create a use-after-free window?

Because devm_pwrseq_device_register() is used in probe, the logical
sequencer device is unregistered by devres after pwrseq_pcie_m2_remove()
returns. 

If an active PCIe or USB consumer (which are not created by this driver and
lack device links) invokes pwrseq_enable() during this window, it will call 
pwrseq_pcie_m2_vregs_enable() which dereferences the freed regulator pointers
inside ctx->regs:

drivers/power/sequencing/pwrseq-pcie-m2.c:pwrseq_pcie_m2_vregs_enable() {
    ...
	return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
}

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