Re: [PATCH v5 2/5] HID: intel-thc-hid: intel-ipts: receive reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: intel-thc-hid: intel-ipts: shared IRQ race unmasks
  thc_ipts_thread()
- [Medium] HID: intel-thc-hid: intel-ipts: DMA buffer leak on
  thc_dma_allocate() failure
--

commit bc2c54076d407e032aad5ea9b6b5fb2d73e4ce2a
Author: Emfox Zhou <emfox@debian.org>

HID: intel-thc-hid: intel-ipts: receive reports

This commit configures the Rx and Tx DMA engines for Intel IPTS ports,
registers a threaded interrupt handler to drain incoming frames, and adds a
work item to recover from transfer errors.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index b0e3fe8ea8aef..76224f4444828 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> @@ -122,6 +132,198 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
[ ... ]
> +static int thc_ipts_dma_start(struct thc_ipts *idev)
> +{
> +	int ret;
> +
> +	/* Allocated once: a start that failed must not leave a NULL buffer. */
> +	if (!idev->rx_frame) {
> +		/* The core rounds the packet size up to a page. */
> +		idev->rx_frame = devm_kzalloc(idev->dev,
> +					      round_up(idev->max_rx, SZ_4K),
> +					      GFP_KERNEL);
> +		if (!idev->rx_frame)
> +			return -ENOMEM;
> +	}
> +
> +	ret = thc_dma_set_max_packet_sizes(idev->thc, 0, idev->max_rx,
> +					   idev->max_tx, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = thc_dma_allocate(idev->thc);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does this leak config->prd_tbls and scatterlist buffers when
thc_dma_allocate() fails?

idev->max_rx is decoded from a 14-bit field (up to 1 MiB) and only checked
against THC_IPTS_RX_HDR_SIZE in thc_ipts_port_start().  If idev->max_rx
exceeds PRD_ENTRIES_NUM * SZ_4K (64 KiB on 4K-page systems) or sgl_alloc()
fails, setup_dma_buffers() returns -ENOMEM after allocating config->prd_tbls
and any earlier config->sgls[i], while thc_dma_allocate()'s rollback skips
the failing channel:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:setup_dma_buffers() {
    ...
	config->prd_tbls = cpu_addr;
	config->prd_tbls_dma_handle = dma_handle;

	buf_sz = dma_get_max_packet_size(dev, config);

	/* Allocate and map the scatter-gather lists, one for each PRD table */
	for (i = 0; i < config->prd_tbl_num; i++) {
		config->sgls[i] = sgl_alloc(buf_sz, GFP_KERNEL, &nent);
		if (!config->sgls[i] || nent > PRD_ENTRIES_NUM) {
			dev_err_once(dev->dev, "sgl_alloc (%uth) failed, nent %u\n",
				     i, nent);
			return -ENOMEM;
		}
    ...
}

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:thc_dma_allocate() {
    ...
release_bufs:
	while (chan--)
		release_dma_buffers(dev, &dev->dma_ctx->dma_config[chan]);

	return ret;
}

Because thc_ipts_dma_start() returns before setting idev->dma_ready,
thc_ipts_dma_stop() also skips thc_dma_release(), leaking the unmanaged
coherent PRD table and scatterlists.

> +
> +	ret = thc_dma_configure(idev->thc);
> +	if (ret) {
> +		thc_dma_release(idev->thc);
> +		return ret;
> +	}
> +
> +	WRITE_ONCE(idev->dma_ready, true);
> +
> +	return 0;
> +}
> +
> +irqreturn_t thc_ipts_hardirq(int irq, void *data)
> +{
> +	struct thc_ipts *idev = data;
> +
> +	/* Level triggered and shared: mask first, or it stays asserted. */
> +	thc_interrupt_enable(idev->thc, false);
> +
> +	/* A reset in flight: the port is masked, so this is not ours. */
> +	if (READ_ONCE(idev->recovering))
> +		return IRQ_NONE;
> +
> +	/* Nothing of ours is pending: the cause belongs to a sharer. */
> +	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready))
> +		return IRQ_NONE;
> +
> +	return IRQ_WAKE_THREAD;
> +}
[ ... ]
> +void thc_ipts_recover(struct work_struct *work)
> +{
> +	struct thc_ipts *idev = container_of(work, struct thc_ipts,
> +					     recover_work);
> +
> +	/* A handler may have unmasked after the error; keep it masked. */
> +	thc_interrupt_enable(idev->thc, false);
> +
> +	if (thc_rxdma_reset(idev->thc)) {
> +		/* Nothing left: keep the port masked until it restarts. */
> +		dev_err(idev->dev, "cannot reset the receive engine\n");
> +		return;
> +	}
> +
> +	WRITE_ONCE(idev->recovering, false);
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +}
> +
> +irqreturn_t thc_ipts_thread(int irq, void *data)
> +{
[ ... ]
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +
> +	return ret;
> +}

[Severity: Medium]
Can a shared interrupt cause thc_ipts_thread() or recovery to run with the
THC port interrupt unmasked?

thc_ipts_probe() requests the interrupt with IRQF_SHARED | IRQF_COND_ONESHOT,
which leaves IRQS_ONESHOT unset unless an earlier sharer requested
IRQF_ONESHOT.  While thc_ipts_thread() is running on CPU0, an interrupt from
another device sharing the INTx line can run thc_ipts_hardirq() on CPU1:

CPU0                                  CPU1
irq_thread()
  thc_ipts_thread()
    for (pass = 0; ...; pass++)
      ...
                                      thc_ipts_hardirq()
                                        thc_interrupt_enable(..., false)
                                        return IRQ_WAKE_THREAD
                                        /* sets IRQTF_RUNTHREAD */
    thc_interrupt_enable(..., true)
  /* sees IRQTF_RUNTHREAD set */
  thc_ipts_thread()
    /* runs with port unmasked */     <-- invariant violated

The second invocation of thc_ipts_thread() runs without thc_ipts_hardirq()
masking the port in between.  In addition, thc_ipts_recover() clears
idev->recovering before calling thc_interrupt_enable(idev->thc, true), and
thc_ipts_dma_start() sets idev->dma_ready before thc_ipts_port_start()
unmasks the port; a shared IRQ in either window can wake thc_ipts_thread()
(and even enter thc_ipts_request_recover()) before the delayed
thc_interrupt_enable(idev->thc, true) re-enables the port interrupt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114234242.177387-1-emfox@debian.org?part=2