Re: [PATCH v5 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

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

New issues:
- [High] Race condition during device removal allows the recovery workqueue item to be scheduled after it was cancelled, leading to a Use-After-Free on `qsdev`.

Pre-existing issues:
- [High] The HID device is destroyed before hardware interrupts are disabled, creating a race condition where the IRQ handler can dereference a freed HID device pointer.
--

--- Patch [3]: [PATCH v5 3/3] HID: Intel-thc-hid: Intel-quickspi: Refine recover callback ---
commit 08604fa17c3b0d7c83e325ba80b11d716e4a7d1a
Author: Even Xu <[email protected]>

HID: Intel-thc-hid: Intel-quickspi: Refine recover callback

This commit refines the recover flow by using a workqueue to handle
recovery instead of processing it in the irq handler. It also calls
thc_rxdma_reset() to simplify the operation and disables interrupts
during the recovery flow.

> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -337,11 +336,10 @@ static irqreturn_t quickspi_irq_thread_handler(int irq, void *dev_id)
>  	}
>  
>  end:
> -	thc_interrupt_enable(qsdev->thc_hw, true);
> -
>  	if (err_recover)
> -		if (try_recover(qsdev))
> -			qsdev->state = QUICKSPI_DISABLED;
> +		schedule_work(&qsdev->recover_work);
> +	else
> +		thc_interrupt_enable(qsdev->thc_hw, true);
>  
>  	pm_runtime_put_autosuspend(qsdev->dev);
>  
[ ... ]
> @@ -461,6 +461,10 @@ static struct quickspi_device *quickspi_dev_init(struct pci_dev *pdev, void __io
>   */
>  static void quickspi_dev_deinit(struct quickspi_device *qsdev)
>  {
> +	WRITE_ONCE(qsdev->recovery_disabled, true);
> +	cancel_work_sync(&qsdev->recover_work);
> +
> +	thc_interrupt_quiesce(qsdev->thc_hw, true);
>  	thc_interrupt_enable(qsdev->thc_hw, false);
>  	thc_ltr_unconfig(qsdev->thc_hw);
>  	thc_wot_unconfig(qsdev->thc_hw);

[Severity: High]
Can this sequence cause a use-after-free on qsdev?

Looking at quickspi_dev_deinit(), the recovery work is cancelled before
hardware interrupts are quiesced and disabled. If a hardware error
interrupt occurs immediately after cancel_work_sync() completes, the
IRQ thread will still run.

In quickspi_irq_thread_handler(), the work is unconditionally scheduled
without checking the recovery_disabled flag:

    if (err_recover)
        schedule_work(&qsdev->recover_work);

If this happens during device removal in quickspi_remove(), devres will
subsequently free qsdev, but the workqueue will execute try_recover()
afterwards and dereference the freed memory.

Could we either check the recovery_disabled flag before scheduling,
or disable interrupts before cancelling the work?

> @@ -711,12 +715,13 @@ static void quickspi_remove(struct pci_dev *pdev)
>  		return;
>  
>  	quickspi_hid_remove(qsdev);
> +
> +	quickspi_dev_deinit(qsdev);
> +
>  	quickspi_dma_deinit(qsdev);
>  
>  	pm_runtime_get_noresume(qsdev->dev);
>  
> -	quickspi_dev_deinit(qsdev);
> -
>  	pci_clear_master(pdev);
>  }

[Severity: High]
This is a pre-existing issue, but can this sequence lead to a
use-after-free of the HID device?

In quickspi_remove(), quickspi_hid_remove() is called to destroy the
HID device before quickspi_dev_deinit() disables hardware interrupts.

If a data interrupt fires in the window between these two calls,
quickspi_irq_thread_handler() will process it:

    if (int_mask & BIT(THC_RXDMA2_INT)) {
        ...
        quickspi_handle_input_data(qsdev, input_len);
    }

Since the HID device has already been freed by quickspi_hid_remove(),
attempting to report input data could dereference a freed pointer.

Should we ensure interrupts are disabled before destroying the HID device?

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