Re: [PATCH] HID: intel-thc-hid: intel-quickspi: fix autosuspend cleanup during teardown

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

Pre-existing issues:
- [High] The hard IRQ handler accesses MMIO hardware registers without verifying that the device is in an active PM state, risking PCIe errors on spurious interrupts.
- [High] The `quickspi_remove` function destroys the HID device and releases DMA resources before disabling the device interrupts and synchronizing the IRQ handler, leading to a Use-After-Free race condition.
--

--- Patch [1]: [PATCH] HID: intel-thc-hid: intel-quickspi: fix autosuspend cleanup during teardown ---
commit c65b63d920ef0da607b2572a6bd252126299f2eb
Author: Guangshuo Li <[email protected]>

HID: intel-thc-hid: intel-quickspi: fix autosuspend cleanup during teardown

This commit fixes a runtime PM reference leak during driver removal. It adds a
missing call to pm_runtime_dont_use_autosuspend() in quickspi_remove() to
balance the initialization done during probe.

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index 4ae2e1718b30c..3c87519aca84a 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -713,6 +713,7 @@ static void quickspi_remove(struct pci_dev *pdev)
>  	quickspi_hid_remove(qsdev);
>  	quickspi_dma_deinit(qsdev);
>  
> +	pm_runtime_dont_use_autosuspend(qsdev->dev);
>  	pm_runtime_get_noresume(qsdev->dev);
>  
>  	quickspi_dev_deinit(qsdev);

[Severity: High]
This isn't a bug introduced by this patch, but does this teardown sequence
create a use-after-free race condition?

During driver removal, quickspi_remove() calls quickspi_dma_deinit() to free
DMA buffers before disabling hardware interrupts in quickspi_dev_deinit().

Because the IRQ was requested with devm_request_threaded_irq(), it remains
active and is not synchronized or freed until after quickspi_remove() returns.

If an interrupt fires during this window, could the concurrently running
quickspi_irq_thread_handler() invoke thc_rxdma_read() and read from the
already freed DMA memory?


While reviewing this file, I noticed another pre-existing issue in the
interrupt handling code:

drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c:quickspi_irq_quick_handler() {
    ...
	/* Disable THC interrupt before current interrupt be handled */
	thc_interrupt_enable(qsdev->thc_hw, false);
    ...
}

[Severity: High]
This is a pre-existing issue, but does the hard IRQ handler access MMIO
hardware registers without verifying that the device is in an active PM state?

If a spurious or shared interrupt occurs while the device is in the PCI D3hot
(runtime suspended) state, the handler calls thc_interrupt_enable() which
accesses MMIO directly without a pm_runtime_get_if_active() check.

Could this read and write powered-down MMIO registers and generate an
Unsupported Request (UR) on the PCIe bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1