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

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

New issues:
- [High] The newly added bounds check for `qsdev->report_len` against `buf_len` is vulnerable to a Time-of-Check to Time-of-Use (TOCTOU) race condition.

Pre-existing issues:
- [Critical] Heap buffer overflow in `write_cmd_to_txdma` due to unbounded userspace input (host to device).
- [High] Missing synchronization around shared DMA buffers leads to data corruption across concurrent HID raw requests.
- [High] Completion flags for report requests are not cleared before sending commands, leading to stale responses.
- [High] Malicious device input can overflow `qsdev->report_buf` due to mismatched maximum bounds (device to host).
- [High] Tearing down DMA before disabling interrupts allows the IRQ thread to access freed scatter-gather lists.
- [High] Missing `dma_sync_sg_for_device` and `dma_sync_sg_for_cpu` calls when accessing long-lived DMA mapped scatterlists.
--

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

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

This patch adds a bounds check to quickspi_get_report() to ensure the
device-supplied qsdev->report_len does not exceed the caller-provided
buf_len. It aims to prevent buffer overflows when copying the response to
the caller's buffer.

> 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);

[Severity: High]
This is a pre-existing issue, but does this code lack synchronization
around shared DMA buffers? 

Multiple userspace threads can concurrently issue requests through
quickspi_hid_raw_request() lacking any mutual exclusion before touching shared
device buffers. Both GET and SET report paths modify and read the same
qsdev->report_buf locklessly (e.g., via write_cmd_to_txdma()), and the IRQ
handler also writes to this buffer. 

Could this lead to data corruption across concurrent HID raw requests?

>  		break;
>  	case HID_REQ_SET_REPORT:
>  		ret = quickspi_set_report(qsdev, rtype, reportnum, buf, len);

[Severity: Critical]
This is a pre-existing issue, but can unbounded userspace input cause a heap
buffer overflow here? 

The user-controlled len is passed to write_cmd_to_txdma(), which executes:

if (report_buf && report_buf_len > 0)
    memcpy(write_buf->content, report_buf, report_buf_len);

This memcpy lacks bounds checking against the allocated size of
qsdev->report_buf, which could allow a local user writing to the hidraw
device to overwrite adjacent heap objects.

> 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..9c4fce09729d3 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
[ ... ]
> @@ -372,6 +373,12 @@ int quickspi_get_report(struct quickspi_device *qsdev,
>  	}
>  	qsdev->get_report_cmpl = false;

[Severity: High]
This is a pre-existing issue, but are the completion flags managed correctly
when timeouts occur? 

If wait_event_interruptible_timeout() times out, it returns early and does not
reset qsdev->get_report_cmpl = false. 

If a delayed interrupt then sets this flag, the next call to
quickspi_get_report() will send a command and immediately pass the wait_event,
consuming stale or uninitialized response data.

>  
> +	if (qsdev->report_len > buf_len) {

[Severity: High]
Is there a Time-of-Check to Time-of-Use (TOCTOU) race condition here? 

The qsdev->report_len variable is evaluated in this condition and then
evaluated again in the memcpy below. Because there is no locking, a
concurrent update from quickspi_handle_input_data() could change
qsdev->report_len between the check and the use, leading to a buffer overflow
in the caller's buffer.

> +		dev_err_once(qsdev->dev, "Get report response too big, %u vs %u\n",
> +			     qsdev->report_len, buf_len);
> +		return -EINVAL;
> +	}
> +
>  	memcpy(buf, qsdev->report_buf, qsdev->report_len);
>  
>  	return qsdev->report_len;
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.h b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.h
> index 775e29c1ed136..8a2338bee8085 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.h
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.h
> @@ -12,7 +12,7 @@ struct quickspi_device;
>  
>  void quickspi_handle_input_data(struct quickspi_device *qsdev, u32 buf_len);

[Severity: High]
This is a pre-existing issue, but could malicious device input overflow
qsdev->report_buf here? 

In quickspi_handle_input_data(), the incoming data is validated against the
size of qsdev->input_buf, but then copied into qsdev->report_buf:

memcpy(qsdev->report_buf, input_report, qsdev->report_len);

Since qsdev->input_buf can be larger than qsdev->report_buf based on varying
device descriptor fields, could a device send a packet that passes the
input_buf bounds check but overflows report_buf?


[Severity: High]
This is a pre-existing issue, but does quickspi_remove() in pci-quickspi.c
tear down DMA resources before disabling interrupts?

quickspi_remove() {
    ...
    quickspi_dma_deinit();
    quickspi_dev_deinit();
}

The driver destroys DMA structures and SG lists before calling
quickspi_dev_deinit() to quiesce hardware interrupts. If a device interrupt
fires in this window, could the IRQ thread access freed scatter-gather
lists?


[Severity: High]
This is a pre-existing issue, but is the driver missing required DMA API sync
calls in intel-thc-dma.c? 

The driver modifies and reads CPU virtual addresses of SG lists using
sg_copy_from_buffer() (in write_dma_buffer()) and sg_copy_to_buffer() (in
thc_swdma_read()). 

Are calls to dma_sync_sg_for_device() and dma_sync_sg_for_cpu() required here
to prevent silent I/O corruption on systems with SWIOTLB or non-coherent DMA
architectures?

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