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

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

Pre-existing issues:
- [Critical] Missing bounds check on untrusted DMA data length in `handle_input_report()` leads to out-of-bounds read and potential crash.
- [High] The driver drops the Runtime PM usage count twice in `quicki2c_probe()`, causing an unbalanced usage count and potential underflow.
- [High] The hard IRQ handler accesses memory-mapped hardware registers without verifying the device is in an active power state.
- [High] The device removal routine destroys the HID device and unconfigures DMA before disabling the hardware interrupt, allowing concurrent access to freed structures (Use-After-Free).
--

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

HID: intel-thc-hid: intel-quicki2c: fix autosuspend cleanup during teardown
    
quicki2c_probe() calls pm_runtime_use_autosuspend(), but
quicki2c_remove() does not call the matching
pm_runtime_dont_use_autosuspend() during teardown.
    
If the autosuspend delay is set to a negative value while autosuspend
is enabled, the runtime PM core increments usage_count to prevent
runtime suspend. Without calling pm_runtime_dont_use_autosuspend()
during teardown, this reference is not dropped.

> 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..4126c2408d986 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -774,6 +774,7 @@ static void quicki2c_remove(struct pci_dev *pdev)
>  	quicki2c_hid_remove(qcdev);
>  	quicki2c_dma_deinit(qcdev);
>  
> +	pm_runtime_dont_use_autosuspend(qcdev->dev);
>  	pm_runtime_get_noresume(qcdev->dev);
>  
>  	quicki2c_dev_deinit(qcdev);

[Severity: High]
This isn't a bug introduced by this patch, but does this teardown sequence
allow a Use-After-Free?

The device removal routine destroys the HID device via quicki2c_hid_remove()
before disabling hardware interrupts in quicki2c_dev_deinit(). If an
interrupt fires in this window, the IRQ thread executes:

quicki2c_irq_thread_handler()
   handle_input_report()
      quicki2c_hid_send_report()

which could access the freed qcdev->hid_dev structure.


drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:quicki2c_probe() {
    ...
	pm_runtime_use_autosuspend(qcdev->dev);
	pm_runtime_set_autosuspend_delay(qcdev->dev, DEFAULT_AUTO_SUSPEND_DELAY_MS);
	pm_runtime_put_noidle(qcdev->dev);
	pm_runtime_put_autosuspend(qcdev->dev);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code drop the PM usage count
twice?

Calling pm_runtime_put_noidle() followed by pm_runtime_put_autosuspend()
drops the usage count twice when it should only be dropped once to balance
the initial count of 1.

This can cause the usage count to underflow and break runtime PM logic.


drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:quicki2c_irq_quick_handler() {
    ...
	if (qcdev->state == QUICKI2C_DISABLED)
		return IRQ_HANDLED;

	/* Disable THC interrupt before current interrupt be handled */
	thc_interrupt_enable(qcdev->thc_hw, false);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this hard IRQ handler
access memory-mapped registers without verifying the device is in an active
power state?

The PCI core can transition the device to D3hot while an in-flight MSI is
arriving.

When quicki2c_irq_quick_handler() executes in this suspended state, blindly
writing to the MMIO register via thc_interrupt_enable() can cause a PCIe
Master Abort, potentially freezing or crashing the system.


drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:handle_input_report() {
    ...
		/* Discard samples before driver probe complete */
		if (qcdev->state != QUICKI2C_ENABLED)
			continue;

		quicki2c_hid_send_report(qcdev, pkt->data,
					 HIDI2C_DATA_LEN(le16_to_cpu(pkt->len)));
	}

	return 0;
}

[Severity: Critical]
This is a pre-existing issue, but does this code lack a bounds check on the
untrusted DMA data length?

The pkt->len field is read directly from DMA memory originating from the
peripheral.

If a malicious or faulty device supplies a len of 1, the HIDI2C_DATA_LEN()
macro calculates 1 - 2, which underflows.

Passing this value to quicki2c_hid_send_report() causes an out-of-bounds read
that can leak memory or cause a kernel panic.

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