Re: [PATCH v2] HID: intel-thc-hid: intel-quickspi: bound GET_REPORT response to the caller buffer

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

New issues:
- [Medium] Missing READ_ONCE() in TOCTOU fix allows compiler to reload value, bypassing the bounds check.

Pre-existing issues:
- [Critical] Missing bounds check on user-supplied `buf_len` in `quickspi_set_report` causes a heap buffer overflow.
- [High] Lack of driver-level serialization allows concurrent IOCTLs and IRQs to corrupt shared DMA buffers.
- [High] Asymmetric teardown in remove function leads to Use-After-Free of HID device in IRQ handler.
- [High] Heap buffer overflow in `read_dma_buffer` due to copying 4K-aligned packet sizes into undersized input buffer.
- [Medium] Failing to reset waitqueue completion flags on timeout causes state machine desynchronization.
--

--- Patch [1]: [PATCH v2] HID: intel-thc-hid: intel-quickspi: bound GET_REPORT response to the caller buffer ---
commit 0324df489da12163fd70da0bf173d44cda181192
Author: HyeongJun An <[email protected]>

HID: intel-thc-hid: intel-quickspi: bound GET_REPORT response to the caller buffer

This commit fixes an out-of-bounds write where the device-supplied response
is copied into the hidraw buffer without checking the buffer's length. It
updates quickspi_get_report() to take the buffer length and validate the
device's report size against it.

Link: https://lore.kernel.org/all/[email protected]/

diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c
index 91d5807b4a830..a60a0a7f16aad 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c
[ ... ]
@@ -61,7 +61,7 @@ static int quickspi_hid_raw_request(struct hid_device *hid,
 
 	switch (reqtype) {
 	case HID_REQ_GET_REPORT:
-		ret = quickspi_get_report(qsdev, rtype, reportnum, buf);
+		ret = quickspi_get_report(qsdev, rtype, reportnum, buf, len);
 		break;
 	case HID_REQ_SET_REPORT:
 		ret = quickspi_set_report(qsdev, rtype, reportnum, buf, len);

[Severity: Critical]
This is a pre-existing issue, but are we missing a bounds check on the
user-supplied len in quickspi_set_report()?

When len (which can be up to HID_MAX_BUFFER_SIZE) is passed down to
write_cmd_to_txdma() in drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:

quickspi_set_report()
   ret = write_cmd_to_txdma(qsdev, rep_type, report_id, buf + 1, buf_len - 1);

Could this unconditionally copy a large user input into the driver's internal
buffer, causing a heap buffer overflow?

>  		break;
[ ... ]
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
index cb19057f1191b..10b06dea39612 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
[ ... ]
@@ -372,9 +374,16 @@ int quickspi_get_report(struct quickspi_device *qsdev,
 	}

[Severity: Medium]
This is a pre-existing issue, but could this timeout error path leave the
completion flag in a desynchronized state?

If wait_event_interruptible_timeout() returns -ETIMEDOUT but the device
later responds, a delayed interrupt could set qsdev->get_report_cmpl to true.
Would the next IOCTL then bypass the wait queue and consume a stale response?

>  	qsdev->get_report_cmpl = false;
>  
> -	memcpy(buf, qsdev->report_buf, qsdev->report_len);
> +	report_len = qsdev->report_len;
> +	if (report_len > buf_len) {
> +		dev_err_once(qsdev->dev, "Get report response too big, %u vs %u\n",
> +			     report_len, buf_len);
> +		return -EINVAL;
> +	}
> +
> +	memcpy(buf, qsdev->report_buf, report_len);

[Severity: Medium]
Is it possible for the compiler to optimize away the local report_len variable
and reload qsdev->report_len directly from memory during the memcpy() call?

If an interrupt updates qsdev->report_len to a larger size after the bounds
check but before the copy, could it bypass the check and overflow the caller's
buffer? Should this use READ_ONCE() to prevent reloading?

> -	return qsdev->report_len;
> +	return report_len;
>  }

[Severity: High]
This is a pre-existing issue, but is there a lack of driver-level serialization
that allows concurrent IOCTLs and IRQs to corrupt shared DMA buffers?

User space threads concurrently calling HIDIOCGFEATURE or HIDIOCSFEATURE via
hidraw only acquire a shared read lock (minors_rwsem). Both
quickspi_get_report() and quickspi_set_report() call write_cmd_to_txdma(),
which writes directly into the shared buffer locklessly:

drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:write_cmd_to_txdma() {
    ...
    if (report_buf)
        memcpy(write_buf->report_bdy, report_buf, report_buf_len);
    ...
}

Furthermore, the threaded IRQ handler concurrently writes device responses into
the exact same buffer:

drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:quickspi_handle_input_data() {
    ...
    memcpy(qsdev->report_buf, input_report, qsdev->report_len);
    ...
}

Can multiple threads and IRQs entering these paths simultaneously cause data
corruption or information leaks?

[Severity: High]
This is a pre-existing issue, but could the asymmetric teardown in
quickspi_remove() lead to a Use-After-Free of the HID device?

quickspi_remove() destroys and frees the hid_dev before hardware interrupts
are disabled:

drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c:quickspi_remove() {
    ...
    quickspi_hid_remove(qsdev);
    quickspi_dma_deinit(qsdev);

    pm_runtime_get_noresume(qsdev->dev);

    quickspi_dev_deinit(qsdev);
    ...
}

If the device is unbound via sysfs while hardware is actively sending
interrupts, could an interrupt fire in this window and cause
quickspi_handle_input_data() to invoke quickspi_hid_send_report(),
accessing the already freed qsdev->hid_dev?

[Severity: High]
This is a pre-existing issue, but is there a heap buffer overflow risk in
read_dma_buffer() due to copying 4K-aligned packet sizes into an undersized
input buffer?

The input_buf is allocated to the exact max_input_len:

drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c:quickspi_alloc_report_buf() {
    ...
    qsdev->input_buf = devm_kzalloc(qsdev->dev, max_input_len, GFP_KERNEL);
    ...
}

However, the bounds check in read_dma_buffer() uses the 4K-aligned
max_packet_size:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:read_dma_buffer() {
    ...
    if (mes_len > read_config->max_packet_size) {
        ...
    }
    ret = sg_copy_to_buffer(sg, nent, read_buff, mes_len);
    ...
}

If a malfunctioning or malicious hardware device sends a packet larger than
max_input_len but within the 4K alignment, will it bypass the bounds check
and overflow the smaller heap allocation?

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