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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race condition during device teardown leading to a Use-After-Free of `qcdev`.
- [Medium] Race condition in resume/thaw paths permanently disables device interrupts.
- [Medium] Premature interrupt unmasking causes an interrupt storm for shared IRQs.
- [Low] Redundant hardware teardown in `quicki2c_poweroff()` triggers kernel warnings.

Pre-existing issues:
- [High] Hardirq handler accesses memory-mapped hardware registers without verifying runtime PM state.
- [High] Unvalidated length field from hardware allows out-of-bounds read in the HID core.
--

--- Patch [2]: [PATCH v4 2/3] HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback ---
commit 8f342c689f3a83c1f792680893d01fe944f899d6
Author: Even Xu <even.xu@intel.com>
Subject: HID: Intel-thc-hid: Intel-quicki2c: Refine recover callback

This commit refines the recover flow by offloading it to a workqueue instead
of processing it in the IRQ handler. It simplifies the operation using the
thc_rxdma_reset() API and ensures interrupts are disabled during the entire
recovery flow.

> 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..3ccbddfe8e5e8 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,31 @@ static irqreturn_t quicki2c_irq_quick_handler(int irq, void *dev_id)
>  }

[Severity: High]
This is a pre-existing issue, but does quicki2c_irq_quick_handler() risk
accessing memory-mapped hardware registers while the device is in a
runtime-suspended state?

The driver allows fallback to shared INTx interrupts, so the hardirq
handler could be invoked by another device on the same IRQ line while this
device is suspended. Calling thc_interrupt_enable() unconditionally could
access powered-off registers and cause Unsupported Requests (UR).

>  
>  /**
> - * try_recover - Try to recovery THC and Device
> - * @qcdev: Pointer to quicki2c_device structure
> + * try_recover - Recover callback to recover THC
> + * @work: pointer to work_struct
>   *
>   * This function is an error handler, called when fatal error happens.
> - * It try to reset touch device and re-configure THC to recovery
> - * communication between touch device and THC.
> - *
> - * Return: 0 if successful or error code on failure
> + * It try to reset Touch Device and re-configure THC to recover
> + * transferring between Device and THC.
>   */
> -static int try_recover(struct quicki2c_device *qcdev)
> +static void try_recover(struct work_struct *work)
>  {
> -	int ret;
> +	struct quicki2c_device *qcdev = container_of(work, struct quicki2c_device, recover_work);
>  
> -	thc_dma_unconfigure(qcdev->thc_hw);
> +	if (READ_ONCE(qcdev->recovery_disabled))
> +		return;
>  
> -	ret = thc_dma_configure(qcdev->thc_hw);
> -	if (ret) {
> -		dev_err(qcdev->dev, "Reconfig DMA failed\n");
> -		return ret;
> -	}
> +	if (pm_runtime_resume_and_get(qcdev->dev))
> +		return;
>  
> -	return 0;
> +	thc_interrupt_enable(qcdev->thc_hw, false);
> +
> +	if (thc_rxdma_reset(qcdev->thc_hw))
> +		qcdev->state = QUICKI2C_DISABLED;
> +	else
> +		thc_interrupt_enable(qcdev->thc_hw, true);
> +
> +	pm_runtime_put_autosuspend(qcdev->dev);
>  }
>  
>  static int handle_input_report(struct quicki2c_device *qcdev)

[Severity: High]
This is a pre-existing issue, but does handle_input_report() trust the
length field from the hardware without validating it?

If a buggy or malicious device sends a packet with pkt->len equal to 1,
HIDI2C_DATA_LEN could underflow to SIZE_MAX when passed to the HID core:

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

Could this unvalidated length cause an out-of-bounds read in the HID core?

[ ... ]

> @@ -343,11 +346,10 @@ static irqreturn_t quicki2c_irq_thread_handler(int irq, void *dev_id)
>  	}
>  
>  exit:
> -	thc_interrupt_enable(qcdev->thc_hw, true);
> -
>  	if (err_recover)
> -		if (try_recover(qcdev))
> -			qcdev->state = QUICKI2C_DISABLED;
> +		schedule_work(&qcdev->recover_work);
> +	else
> +		thc_interrupt_enable(qcdev->thc_hw, true);

[Severity: Medium]
Could this unconditionally re-enable the hardware interrupt while a recovery
is already pending?

If err_recover is 0 (for instance, if another shared interrupt fires), the
else branch unmasks the broken device's interrupt prematurely.

Does this cause an interrupt storm before try_recover() has a chance to run?

[ ... ]

> @@ -439,6 +443,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);
>  	thc_ltr_unconfig(qcdev->thc_hw);

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

cancel_work_sync() is called before the hardware interrupts are quiesced
and disabled. If the threaded IRQ handler is preempted right before calling
schedule_work(), it could queue recover_work after cancel_work_sync()
completes:

CPU1                                   CPU2
quicki2c_irq_thread_handler()          quicki2c_dev_deinit()
   err_recover = 1;
   // preempted
                                       cancel_work_sync();
                                       thc_interrupt_quiesce();
                                       // device removed, qcdev freed
   schedule_work(&qcdev->recover_work);

When try_recover() eventually runs, will it dereference the freed qcdev
pointer?

[ ... ]

> @@ -867,6 +878,8 @@ static int quicki2c_resume(struct device *device)
>  	if (ret)
>  		return ret;
>  
> +	WRITE_ONCE(qcdev->recovery_disabled, false);
> +

[Severity: Medium]
Does unmasking the interrupt before setting recovery_disabled to false
create a window where the device can become permanently disabled?

If the hardware asserts an interrupt after thc_interrupt_quiesce() enables
it, but before WRITE_ONCE(qcdev->recovery_disabled, false) runs, the IRQ
handler will queue the recovery work.

The work will execute try_recover(), read recovery_disabled as true, and
return immediately without re-enabling interrupts. Will this leave the
device permanently unresponsive?

>  	if (!device_may_wakeup(qcdev->dev))
>  		return quicki2c_set_power(qcdev, HIDI2C_ON);

[ ... ]

> @@ -938,6 +956,8 @@ static int quicki2c_poweroff(struct device *device)
>  
>  	thc_ltr_unconfig(qcdev->thc_hw);
>  
> +	quicki2c_dev_deinit(qcdev);
> +
>  	quicki2c_dma_deinit(qcdev);
>  
>  	return 0;

[Severity: Low]
Does adding quicki2c_dev_deinit() here result in redundant hardware teardown
steps?

quicki2c_poweroff() manually calls thc_interrupt_quiesce() and
thc_ltr_unconfig() just before this line. Since quicki2c_dev_deinit() also
calls those functions, it looks like they are executed twice, which might
trigger kernel warnings from the underlying hardware functions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717035609.3874015-1-even.xu@intel.com?part=2