* Re: [PATCH] HID: intel-thc-hid: intel-quicki2c: size the input buffer for the DMA
2026-08-22 12:45 [PATCH] HID: intel-thc-hid: intel-quicki2c: size the input buffer for the DMA HyeongJun An
@ 2026-08-22 13:05 ` sashiko-bot
2026-08-26 1:10 ` Xu, Even
1 sibling, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-08-22 13:05 UTC (permalink / raw)
To: HyeongJun An; +Cc: linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
Pre-existing issues:
- [High] Information Leak / Stale Data Use via unverified DMA read length in `quicki2c_get_report`.
- [High] Memory leak of DMA buffers on the `quicki2c_probe()` error path.
- [High] Use-After-Free of `hid_device` due to missing synchronization during device unbind.
- [High] Use-After-Free of `qcdev` via `recover_work` workqueue during device unbind.
- [High] Data corruption due to concurrent lockless accesses to the shared `input_buf`.
- [High] DMA buffer overrun vulnerability on `qsdev->input_buf` in sister driver `intel-quickspi`.
- [High] Out-of-Bounds Read in `handle_input_report` due to trusting hardware length field and unsafe HID API.
- [Medium] Data races due to unprotected state modifications of `qcdev->state`.
--
--- Patch [1]: [PATCH] HID: intel-thc-hid: intel-quicki2c: size the input buffer for the DMA ---
commit bd435db6ebf25c331234086da4dfea64fbc2e7e3
Author: HyeongJun An <sammiee5311@gmail.com>
HID: intel-thc-hid: intel-quicki2c: size the input buffer for the DMA
This commit sizes the input buffer based on the SWDMA packet size while
keeping a 4K floor. This prevents DMA buffer overruns when devices declare a
larger report length than their max input length.
> 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 0d2ad7bc3648a..f6f9f95296d36 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -602,9 +602,12 @@ static int quicki2c_alloc_report_buf(struct quicki2c_device *qcdev)
>
> /*
> * Some HIDI2C devices don't declare input/output max length correctly,
> - * give default 4K buffer to avoid DMA buffer overrun.
> + * give default 4K buffer to avoid DMA buffer overrun. Both RxDMA2 and
> + * SWDMA land here, so cover the larger SWDMA packet size.
> */
> - max_report_len = max(le16_to_cpu(qcdev->dev_desc.max_input_len), SZ_4K);
> + max_report_len = max(le16_to_cpu(qcdev->dev_desc.max_input_len),
> + le16_to_cpu(qcdev->dev_desc.report_desc_len));
> + max_report_len = max_t(size_t, ALIGN(max_report_len, SZ_4K), SZ_4K);
>
> qcdev->input_buf = devm_kzalloc(qcdev->dev, max_report_len, GFP_KERNEL);
[Severity: High]
This is a pre-existing issue, but does this same vulnerability still exist
in the sister driver intel-quickspi?
Looking at quickspi_alloc_report_buf() in pci-quickspi.c:
max_input_len = max(le16_to_cpu(qsdev->dev_desc.rep_desc_len),
le16_to_cpu(qsdev->dev_desc.max_input_len));
qsdev->input_buf = devm_kzalloc(qsdev->dev, max_input_len, GFP_KERNEL);
Since the DMA hardware is instructed elsewhere that it can safely write up to
a 4K-aligned size, could a device sending more data than declared overrun
this unaligned allocation, just like the quicki2c bug being fixed here?
[Severity: High]
This is a pre-existing issue, but does the shared input_buf suffer from
concurrent lockless accesses?
Process context uses it during SWDMA transfers in quicki2c_get_report():
rpt = (struct hidi2c_report_packet *)qcdev->input_buf;
ret = thc_swdma_read(qcdev->thc_hw, qcdev->report_buf, len, NULL, rpt, &read_len);
Meanwhile, the interrupt context unconditionally reads into it without locks
in handle_input_report():
struct hidi2c_report_packet *pkt = (struct hidi2c_report_packet *)qcdev->input_buf;
...
ret = thc_rxdma_read(qcdev->thc_hw, THC_RXDMA2, (u8 *)pkt, &report_len,
If a hardware input event arrives during a synchronous get_report ioctl,
could this corrupt the command response?
[Severity: High]
This is a pre-existing issue, but could this shared input buffer leak
uninitialized or stale data back to userspace?
In quicki2c_get_report(), the read_len populated by thc_swdma_read() seems
to be ignored:
ret = thc_swdma_read(qcdev->thc_hw, qcdev->report_buf, len, NULL, rpt, &read_len);
if (ret) {
...
if (HIDI2C_DATA_LEN(le16_to_cpu(rpt->len)) != buf_len || rpt->data[0] != reportnum) {
...
memcpy(buf, rpt->data, buf_len);
If a short DMA read or 0-byte read succeeds, could the stale data remaining
in input_buf coincidentally match the expected lengths and report ID,
causing memcpy to copy stale kernel memory?
[Severity: High]
This is a pre-existing issue, but does the input report handling trust the
hardware length field without validating it against the actual DMA transfer
size?
In handle_input_report(), it passes the packet length directly:
quicki2c_hid_send_report(qcdev, pkt->data,
HIDI2C_DATA_LEN(le16_to_cpu(pkt->len)));
Which then gets passed to the HID core in quicki2c_hid_send_report():
ret = hid_input_report(qcdev->hid_dev, HID_INPUT_REPORT, data, data_len, 1);
Since hid_input_report() lacks the buffer boundary checks of
hid_safe_input_report(), could malicious hardware providing a small DMA
transfer but a large forged pkt->len cause an out-of-bounds read?
[Severity: High]
This is a pre-existing issue, but does the device probe error path leak the
DMA buffers?
In quicki2c_probe():
ret = thc_interrupt_quiesce(qcdev->thc_hw, false);
if (ret)
goto dev_deinit;
...
dma_deinit:
quicki2c_dma_deinit(qcdev);
dev_deinit:
quicki2c_dev_deinit(qcdev);
Does jumping directly to dev_deinit bypass the quicki2c_dma_deinit()
cleanup?
[Severity: High]
This is a pre-existing issue, but is there a missing synchronization step
during device unbind that could lead to a use-after-free?
In quicki2c_remove():
quicki2c_hid_remove(qcdev);
quicki2c_dev_deinit(qcdev);
If a hardware interrupt fires after the HID device is destroyed but before
quicki2c_dev_deinit() disables the hardware interrupts, could the IRQ thread
execute this in quicki2c_hid_send_report():
ret = hid_input_report(qcdev->hid_dev, HID_INPUT_REPORT, data, data_len, 1);
And dereference the freed qcdev->hid_dev pointer?
[Severity: High]
This is a pre-existing issue, but could the recovery workqueue cause a
use-after-free during device unbind?
In quicki2c_dev_deinit():
WRITE_ONCE(qcdev->recovery_disabled, true);
cancel_work_sync(&qcdev->recover_work);
thc_interrupt_quiesce(qcdev->thc_hw, true);
Since thc_interrupt_quiesce() disables the hardware interrupts after the
work is canceled, could an interrupt fire in between and blindly schedule
the work again?
In quicki2c_irq_thread_handler():
exit:
if (err_recover)
schedule_work(&qcdev->recover_work);
Could this delayed work then execute on the freed qcdev memory after the
unbind completes?
[Severity: Medium]
This is a pre-existing issue, but are there data races when modifying the
device state?
The state is modified locklessly in a workqueue in try_recover():
qcdev->state = QUICKI2C_DISABLED;
And read locklessly in the IRQ thread in handle_input_report():
if (qcdev->state != QUICKI2C_ENABLED)
continue;
Could this lead to torn reads or inconsistent state logic due to compiler
reordering?
> if (!qcdev->input_buf)
> return -ENOMEM;
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260822124531.644834-1-sammiee5311@gmail.com?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] HID: intel-thc-hid: intel-quicki2c: size the input buffer for the DMA
2026-08-22 12:45 [PATCH] HID: intel-thc-hid: intel-quicki2c: size the input buffer for the DMA HyeongJun An
2026-08-22 13:05 ` sashiko-bot
@ 2026-08-26 1:10 ` Xu, Even
2026-08-26 12:03 ` HyeongJun An
1 sibling, 1 reply; 5+ messages in thread
From: Xu, Even @ 2026-08-26 1:10 UTC (permalink / raw)
To: HyeongJun An, Sun, Xinpeng, Jiri Kosina, Benjamin Tissoires
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org
> -----Original Message-----
> From: HyeongJun An <sammiee5311@gmail.com>
> Sent: Saturday, August 22, 2026 8:46 PM
> To: Xu, Even <even.xu@intel.com>; Sun, Xinpeng <xinpeng.sun@intel.com>; Jiri
> Kosina <jikos@kernel.org>; Benjamin Tissoires <bentiss@kernel.org>
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org; HyeongJun An <sammiee5311@gmail.com>
> Subject: [PATCH] HID: intel-thc-hid: intel-quicki2c: size the input buffer for the
> DMA
>
> quicki2c_alloc_report_buf() sizes input_buf as max(max_input_len, SZ_4K), but
> two channels deliver into it and dma_set_max_packet_size() rounds both
> up: RxDMA2 to ALIGN(max_input_len, SZ_4K) and SWDMA to
> ALIGN(max(max_input_len, report_desc_len), SZ_4K). read_dma_buffer() bounds
> the copy against those, not against the allocation.
Hi, HyeongJun,
It's not a problem.
There are two layers buffer in THC driver:
one is in thc-hw driver for HW DMA, which initialized in quicki2c_dma_init() by thc_dma_set_max_packet_sizes();
one is in quicki2c driver for HID, which allocated in quicki2c_alloc_report_buf().
SWDMA will use the buffer and size initialized by thc_dma_set_max_packet_sizes(), it's already the max one, so don't worry.
>
> quicki2c_get_report() reads into input_buf with prd_tbl_len NULL, so nothing
> programs a length, and a device declaring report_desc_len 5000 with
> max_input_len 64 overruns the 4K buffer by 4096 bytes. The RxDMA2 leg
> additionally needs the I2C max input size clamp to be off.
Quicki2c driver has different buffers for different hid report separately, so input report and report descriptor have different buffer with different size.
>
> Size input_buf from the SWDMA packet size and keep the 4K floor.
>
> Fixes: 66b59bfce6d9 ("HID: intel-thc-hid: intel-quicki2c: Complete THC QuickI2C
> driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
> ---
> Not reproduced on hardware. Triggering it needs a controller that sends more
> than the lengths it declared, which is the case the existing comment was already
> written for.
>
> max_report_len is reused for the report_buf allocation below, so that buffer and
> qcdev->report_len grow with input_buf whenever the SWDMA ceiling is the
> larger term. Both stay bounded by that ceiling.
SWDMA can be used for report descriptor retrieve and manual input report read,
quicki2c driver already set SWDMA buffer to max(le16_to_cpu(qcdev->dev_desc.max_input_len), le16_to_cpu(qcdev->dev_desc.report_desc_len));
so it's not a problem, that's why you never reproduce the issue.
>
> drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> 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 0d2ad7bc3648..f6f9f95296d3 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -602,9 +602,12 @@ static int quicki2c_alloc_report_buf(struct
> quicki2c_device *qcdev)
>
> /*
> * Some HIDI2C devices don't declare input/output max length correctly,
> - * give default 4K buffer to avoid DMA buffer overrun.
> + * give default 4K buffer to avoid DMA buffer overrun. Both RxDMA2 and
> + * SWDMA land here, so cover the larger SWDMA packet size.
> */
> - max_report_len = max(le16_to_cpu(qcdev->dev_desc.max_input_len),
> SZ_4K);
> + max_report_len = max(le16_to_cpu(qcdev->dev_desc.max_input_len),
> + le16_to_cpu(qcdev->dev_desc.report_desc_len));
> + max_report_len = max_t(size_t, ALIGN(max_report_len, SZ_4K), SZ_4K);
>
> qcdev->input_buf = devm_kzalloc(qcdev->dev, max_report_len,
> GFP_KERNEL);
> if (!qcdev->input_buf)
> --
> 2.43.0
^ permalink raw reply [flat|nested] 5+ messages in thread