Re: [PATCH v8 14/16] power: sequencing: pcie-m2: Split Bluetooth unit based on interface
From: sashiko-bot@kernel.org
Date: 2026-09-04 09:08:01
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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