Re: [PATCH v2 2/2] Bluetooth: btintel_pcie: Fix TOCTOU race in reset path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Kernel panic due to unconditionally dereferencing a NULL `hdev` pointer during PM resume if a prior reset failed.
- [High] Non-atomic modification of data->flags causes data race with atomic bit operations, risking lost coredumps and permanent lockouts.
- [High] Unprotected assignment to dev_data->reset_type in btintel_pcie_hw_error can corrupt an ongoing reset.
--

commit c9025f5524eb70c6235927f87e97777af4f0b9ab
Author: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>

Bluetooth: btintel_pcie: Fix TOCTOU race in reset path

This commit fixes a Time-Of-Check to Time-Of-Use race condition in the
reset path of the btintel_pcie driver. It reorders the RECOVERY_IN_PROGRESS
atomic guard to execute before checking the SETUP_DONE flag, preventing
concurrent reset requests from racing.

> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index d3a03cf964216..f4f400421833b 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -2686,11 +2686,15 @@ static void btintel_pcie_reset(struct hci_dev *hdev, u8 reset_type)
>  
>  	data = hci_get_drvdata(hdev);

[Severity: Critical]
This is a pre-existing issue, but can this dereference cause a kernel panic
during PM resume if a prior reset failed?

If a reset fails (e.g., in btintel_pcie_setup_hdev()), the device is
unregistered and data->hdev is left permanently set to NULL. If the system
goes into a suspend/hibernate cycle, the resume handler unconditionally
calls this reset function:

btintel_pcie_resume()
    set_bit(BTINTEL_PCIE_CORE_HALTED, &data->flags);
    btintel_pcie_reset(data->hdev, 0);

When btintel_pcie_reset() runs, it calls hci_get_drvdata(hdev), which blindly
dereferences hdev. Should there be a NULL pointer check for hdev here?

> -	if (!test_bit(BTINTEL_PCIE_SETUP_DONE, &data->flags))
> +	if (test_and_set_bit(BTINTEL_PCIE_RECOVERY_IN_PROGRESS, &data->flags)) {
> +		bt_dev_warn(hdev, "Reset rejected: recovery already in progress");
>  		return;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying atomic
flags operations, I noticed a potential data race with data->flags in the
worker thread:

btintel_pcie_reset_work()
    btintel_pcie_start_rx(data);
    data->flags = 0;
    err = btintel_pcie_setup_hdev(data);

Because data->flags is cleared non-atomically immediately after interrupts are
re-enabled, could this erase an atomic modification from an incoming MSI-X
interrupt, such as test_and_set_bit(BTINTEL_PCIE_HWEXP_INPROGRESS, &data->flags)?

Would it be safer to clear the specific bits using atomic operations instead?

> -	if (test_and_set_bit(BTINTEL_PCIE_RECOVERY_IN_PROGRESS, &data->flags))
> +	if (!test_bit(BTINTEL_PCIE_SETUP_DONE, &data->flags)) {
> +		clear_bit(BTINTEL_PCIE_RECOVERY_IN_PROGRESS, &data->flags);
>  		return;
> +	}
>  
>  	data->reset_type = (reset_type == 1) ?
>  			    BTINTEL_PCIE_IOSF_PRR_PLDR : BTINTEL_PCIE_IOSF_PRR_FLR;

[Severity: High]
This is a pre-existing issue, but I have a question about how the shared
reset_type state is handled concurrently.

In the hardware error handler, dev_data->reset_type is overwritten outside
of any lock before the atomic guard in btintel_pcie_reset() is reached:

btintel_pcie_hw_error()
    if (code == 0x13)
        dev_data->reset_type = BTINTEL_PCIE_IOSF_PRR_PLDR;
    else
        dev_data->reset_type = BTINTEL_PCIE_IOSF_PRR_FLR;
        
    btintel_pcie_reset(hdev, ...);

If btintel_pcie_reset_work() is actively executing a recovery and reads
data->reset_type locklessly, could this unprotected assignment cause the
worker thread to observe an inconsistent state and corrupt the recovery
sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618085016.9173-1-chandrashekar.devegowda@intel.com?part=2