Re: [PATCH] HID: intel-thc-hid: intel-quicki2c: size the input buffer for the DMA

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 <[email protected]>

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/[email protected]?part=1