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

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

New issues:
- [High] Use-After-Free and race condition during device teardown due to improper workqueue cancellation.
- [Medium] Failed suspend operation leaves the device recovery mechanism permanently disabled.

Pre-existing issues:
- [High] Missing cleanup of IRQ vectors on error paths and device removal, leading to an IRQ resource leak.
- [High] Unsafe MMIO register access in hard IRQ handler without PM runtime verification.
- [High] Use-After-Free on the HID device structure during removal.
--

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

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

This commit introduces a workqueue for the recovery flow in the quicki2c
driver instead of processing it in the IRQ handler. It also simplifies
recovery using the thc_rxdma_reset() API and disables interrupts during
the whole recover flow to prevent storms on failure.

> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> index 46d3e9a019995..539da002aa4fd 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -245,28 +245,33 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
>  }

[Severity: High]
This is a pre-existing issue, but could this cause an unsafe MMIO access?

Looking at quicki2c_irq_quick_handler():

static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
{
    ...
    /* Disable THC interrupt before current interrupt be handled */
    thc_interrupt_enable(qcdev->thc_hw, false);
    ...
}

The probe function allows fallback to shared legacy INTx interrupts via
pci_alloc_irq_vectors(). If the device is in D3hot (runtime suspended) and
a shared interrupt triggers on the same line, could this handler access the
thc_interrupt_enable MMIO register on powered-down hardware?

PM subsystem guidelines suggest checking pm_runtime_get_if_active() before
register access in shared IRQ handlers.

[ ... ]

> @@ -439,6 +445,9 @@ static struct quicki2c_device *quicki2c_dev_init(struct pci_dev *pdev, void __io
>   */
>  static void quicki2c_dev_deinit(struct quicki2c_device *qcdev)
>  {
> +	WRITE_ONCE(qcdev->recovery_disabled, true);
> +	cancel_work_sync(&qcdev->recover_work);
> +
>  	thc_interrupt_quiesce(qcdev->thc_hw, true);
>  	thc_interrupt_enable(qcdev->thc_hw, false);

[Severity: High]
Is there a race condition here that could lead to a use-after-free?

Because cancel_work_sync() is called before the hardware interrupt source
is disabled with thc_interrupt_enable(), could a concurrent or preempted
threaded IRQ handler call schedule_work() immediately after
cancel_work_sync() returns?

If the work is placed back on the queue and the qcdev structure is
subsequently freed during removal, would the workqueue eventually execute
try_recover() on freed memory?

[ ... ]

> @@ -772,12 +781,13 @@ static void quicki2c_remove(struct pci_dev *pdev)
>  		return;
>  
>  	quicki2c_hid_remove(qcdev);
> +
> +	quicki2c_dev_deinit(qcdev);

[Severity: High]
This is a pre-existing issue, but does destroying the HID device before
disabling hardware interrupts create a race window?

If an RX interrupt arrives after quicki2c_hid_remove() completes but before
quicki2c_dev_deinit() disables interrupts, could the threaded IRQ handler
execute handle_input_report() and invoke quicki2c_hid_send_report(),
leading to a dereference of the freed qcdev->hid_dev pointer?

[Severity: High]
This is also a pre-existing issue, but are the IRQ vectors leaked
during removal?

The probe function allocates vectors via pci_alloc_irq_vectors() with
PCI_IRQ_ALL_TYPES, but there does not appear to be a corresponding
pci_free_irq_vectors() in quicki2c_remove() or the probe error paths.

Does this result in a permanent leak of system IRQ resources?

>  	quicki2c_dma_deinit(qcdev);
>  
>  	pm_runtime_get_noresume(qcdev->dev);
>  
> -	quicki2c_dev_deinit(qcdev);
> -
>  	pci_clear_master(pdev);
>  }

[ ... ]

> @@ -826,6 +836,9 @@ static int quicki2c_suspend(struct device *device)
>  	if (ret)
>  		return ret;
>  
> +	WRITE_ONCE(qcdev->recovery_disabled, true);
> +	cancel_work_sync(&qcdev->recover_work);
> +
>  	ret = thc_interrupt_quiesce(qcdev->thc_hw, true);
>  	if (ret)
>  		return ret;

[Severity: Medium]
If thc_interrupt_quiesce() fails and returns an error here, is
qcdev->recovery_disabled left permanently set to true?

Since a failed suspend aborts the transition and won't trigger a subsequent
resume callback to reset the flag, would this leave the driver in a state
where try_recover() silently aborts on all future errors?

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