Re: [PATCH 2/2] HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to report_buf

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] State machine desynchronization in raw request handlers due to unreset completion flags.
- [High] Missing locking in raw request handlers allows concurrent requests to corrupt the shared command and response buffers.
- [High] Use-After-Free: DMA buffers are freed during driver remove() without synchronizing with the threaded IRQ handler.
- [Medium] Subsystem API Violation: The transport driver uses hid_input_report() instead of hid_safe_input_report().
- [Medium] Signal interruptions are incorrectly treated as timeouts in raw requests, breaking signal handling.
--

--- Patch [2]: [PATCH 2/2] HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to report_buf ---
commit c4e25ca5cbdc0b92b00b6a64c016c4b8ee39d40d
Author: HyeongJun An <sammiee5311@gmail.com>

HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to report_buf

This patch bounds the GET_FEATURE and GET_INPUT_REPORT response copy lengths
to qsdev->report_buf_size. It drops responses that exceed this size to prevent
overflowing the buffer with responses bounded only by the DMA packet size.

> 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 847c5ec55569a..7d34cc22a11a8 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
[ ... ]
> @@ -210,10 +211,17 @@ void quickspi_handle_input_data(struct quickspi_device *qsdev, u32 buf_len)
>  
>  	case GET_FEATURE_RESPONSE:
>  	case GET_INPUT_REPORT_RESPONSE:
> -		qsdev->report_len = sizeof(body_hdr->content_id) + input_len;
> +		report_len = sizeof(body_hdr->content_id) + input_len;
> +		if (report_len > qsdev->report_buf_size) {
> +			dev_err_once(qsdev->dev, "Get report response too big: %u\n",
> +				     report_len);
> +			return;
> +		}
> +
> +		qsdev->report_len = report_len;
>  		input_report = input_body->content - sizeof(body_hdr->content_id);
>  
> -		memcpy(qsdev->report_buf, input_report, qsdev->report_len);
> +		memcpy(qsdev->report_buf, input_report, report_len);

[Severity: High]
This is a pre-existing issue, but missing locking in the raw request handlers 
appears to allow concurrent requests to corrupt qsdev->report_buf.

The HID core does not serialize raw_request callbacks. If multiple threads 
concurrently call quickspi_hid_raw_request(), they will both execute 
write_cmd_to_txdma(). 

This function writes the outgoing command into a single, unprotected shared 
buffer qsdev->report_buf:

quickspi_hid_raw_request() -> write_cmd_to_txdma() {
    ...
    write_buf = (struct output_report *)qsdev->report_buf;
    ...
    memcpy(write_buf->content, report_buf, report_buf_len);
}

Concurrently, the IRQ handler quickspi_handle_input_data() also writes the 
incoming response directly into the exact same qsdev->report_buf here.

Without a mutex protecting the request-response cycle and this buffer, could 
concurrent requests overwrite each other's outgoing commands, and could 
incoming responses overwrite commands being constructed?


[Severity: High]
This is a pre-existing issue, but can the state machine become desynchronized 
if a raw request times out?

If quickspi_get_report() times out, it returns -ETIMEDOUT without resetting 
the completion flag qsdev->get_report_cmpl:

quickspi_get_report() {
    ...
    ret = wait_event_interruptible_timeout(qsdev->get_report_cmpl_wq,
                                           qsdev->get_report_cmpl,
                                           QUICKSPI_ACK_WAIT_TIMEOUT * HZ);
    ...
}

If the hardware eventually sends a delayed response, the IRQ handler will 
set the flag to true. When a subsequent raw request is issued, it will write 
the new command, but wait_event_interruptible_timeout() will return 
immediately because the flag is already true.

Would this cause the driver to return stale data from the previous request 
and lose synchronization with the device? 

The set_report_cmpl flag appears to suffer from the exact same 
desynchronization flaw in quickspi_set_report().


[Severity: Medium]
This is a pre-existing issue, but are signal interruptions incorrectly treated 
as timeouts in raw requests?

In quickspi_get_report() and quickspi_set_report(), the driver uses 
wait_event_interruptible_timeout(). If the wait is interrupted by a signal, 
the function returns a negative error code (such as -ERESTARTSYS).

The driver checks for ret <= 0 and unconditionally returns -ETIMEDOUT:

quickspi_get_report() {
    ...
    if (ret <= 0 || !qsdev->get_report_cmpl) {
        dev_err_once(qsdev->dev, "Wait Get Report Response timeout, ret:%d\n", ret);
        return -ETIMEDOUT;
    }
    ...
}

Does this swallow the actual signal error code, preventing userspace from 
correctly handling signals and breaking the syscall restart mechanism?


[Severity: Medium]
This is a pre-existing issue, but should the transport driver use 
hid_safe_input_report() instead of hid_input_report() here?

According to the HID subsystem documentation, transport drivers must prefer 
hid_safe_input_report() over hid_input_report() when feeding data from 
hardware interrupts to the HID core. 

This allows the core to perform strict boundary checks against the maximum 
allocated buffer size, preventing out-of-bounds reads if a device supplies 
a malicious report descriptor.

quickspi_hid_send_report() {
    ...
    ret = hid_input_report(qsdev->hid_dev, HID_INPUT_REPORT, data, data_len, 1);
    ...
}

Does using hid_input_report() expose the HID core to potential out-of-bounds 
reads?


[Severity: High]
This is a pre-existing issue, but could DMA buffers be freed during driver 
remove() without synchronizing with the threaded IRQ handler?

During driver removal, quickspi_remove() disables hardware interrupts and 
then calls quickspi_dma_deinit() which releases the DMA buffers:

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

However, it does not seem to synchronize with the devm-managed threaded IRQ 
handler (quickspi_irq_thread_handler), which might already be executing on 
another CPU. 

The threaded handler calls thc_rxdma_read(), which reads from the DMA 
buffers:

quickspi_irq_thread_handler() {
    ...
    ret = thc_rxdma_read(qsdev->thc_hw, THC_RXDMA2, qsdev->input_buf,
                         &input_len, &read_finished);
    ...
}

If thc_dma_release() frees these buffers while the threaded handler is 
concurrently executing, could this result in a Use-After-Free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820111558.475304-1-sammiee5311@gmail.com?part=2