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

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

[HyeongJun An]

quickspi_hid_raw_request() receives the caller's buffer length in len, but
quickspi_get_report() never sees it and copies the whole device-supplied
response into buf regardless:

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

qsdev->report_len comes from the input report the touch controller returns,
while buf is sized to whatever the caller asked hidraw for through
HIDIOCGFEATURE or HIDIOCGINPUT.  A response larger than that overflows buf
with device-controlled content.

The intel-quicki2c sibling already passes the caller length down to
quicki2c_get_report() and validates the response against it before the
copy.  Do the same here.

Fixes: 4138f21115ae ("HID: intel-thc-hid: intel-quickspi: Complete THC QuickSPI driver")
Suggested-by: Sashiko AI <sashiko-bot@kernel.org>
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
 drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c  | 2 +-
 .../hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c | 9 ++++++++-
 .../hid/intel-thc-hid/intel-quickspi/quickspi-protocol.h | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

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 91d5807b4a83..a60a0a7f16aa 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);
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 cb19057f1191..9c4fce09729d 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
@@ -342,7 +342,8 @@ int reset_tic(struct quickspi_device *qsdev)
 }
 
 int quickspi_get_report(struct quickspi_device *qsdev,
-			u8 report_type, unsigned int report_id, void *buf)
+			u8 report_type, unsigned int report_id, void *buf,
+			u32 buf_len)
 {
 	int rep_type;
 	int ret;
@@ -372,6 +373,12 @@ int quickspi_get_report(struct quickspi_device *qsdev,
 	}
 	qsdev->get_report_cmpl = false;
 
+	if (qsdev->report_len > buf_len) {
+		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 775e29c1ed13..8a2338bee808 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);
 int quickspi_get_report(struct quickspi_device *qsdev, u8 report_type,
-			unsigned int report_id, void *buf);
+			unsigned int report_id, void *buf, u32 buf_len);
 int quickspi_set_report(struct quickspi_device *qsdev, u8 report_type,
 			unsigned int report_id, void *buf, u32 buf_len);
 int quickspi_get_report_descriptor(struct quickspi_device *qsdev);
-- 
2.43.0

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

[sashiko-bot]

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 <sammiee5311@gmail.com>

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/20260806133838.3378362-1-sammiee5311@gmail.com?part=1

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

[HyeongJun An]

quickspi_hid_raw_request() receives the caller's buffer length in len, but
quickspi_get_report() never sees it and copies the whole device-supplied
response into buf regardless:

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

qsdev->report_len comes from the input report the touch controller returns,
while buf is sized to whatever the caller asked hidraw for through
HIDIOCGFEATURE or HIDIOCGINPUT.  A response larger than that overflows buf
with device-controlled content.

The intel-quicki2c sibling already passes the caller length down to
quicki2c_get_report() and validates the response against it before the
copy.  Do the same here.

Fixes: 4138f21115ae ("HID: intel-thc-hid: intel-quickspi: Complete THC QuickSPI driver")
Suggested-by: Sashiko AI <sashiko-bot@kernel.org>
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
v2: fixed a Time-of-Check-to-Time-of-Use race the Sashiko AI review of v1
    pointed out - the new bound read qsdev->report_len once for the check
    and again for the memcpy, so a concurrent update between the two could
    have overflowed buf with the checked-against-a-smaller-value length.
    Read it once into a local and use that for both.  No other change.

v1: https://lore.kernel.org/all/20260806133838.3378362-1-sammiee5311@gmail.com/

 .../intel-thc-hid/intel-quickspi/quickspi-hid.c   |  2 +-
 .../intel-quickspi/quickspi-protocol.c            | 15 ++++++++++++---
 .../intel-quickspi/quickspi-protocol.h            |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

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 91d5807b4a83..a60a0a7f16aa 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);
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 cb19057f1191..10b06dea3961 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
@@ -342,10 +342,12 @@ int reset_tic(struct quickspi_device *qsdev)
 }
 
 int quickspi_get_report(struct quickspi_device *qsdev,
-			u8 report_type, unsigned int report_id, void *buf)
+			u8 report_type, unsigned int report_id, void *buf,
+			u32 buf_len)
 {
 	int rep_type;
 	int ret;
+	u32 report_len;
 
 	if (report_type == HID_INPUT_REPORT) {
 		rep_type = GET_INPUT_REPORT;
@@ -372,9 +374,16 @@ int quickspi_get_report(struct quickspi_device *qsdev,
 	}
 	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);
 
-	return qsdev->report_len;
+	return report_len;
 }
 
 int quickspi_set_report(struct quickspi_device *qsdev,
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 775e29c1ed13..8a2338bee808 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);
 int quickspi_get_report(struct quickspi_device *qsdev, u8 report_type,
-			unsigned int report_id, void *buf);
+			unsigned int report_id, void *buf, u32 buf_len);
 int quickspi_set_report(struct quickspi_device *qsdev, u8 report_type,
 			unsigned int report_id, void *buf, u32 buf_len);
 int quickspi_get_report_descriptor(struct quickspi_device *qsdev);
-- 
2.43.0

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

[sashiko-bot]

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 <sammiee5311@gmail.com>

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/20260806133838.3378362-1-sammiee5311@gmail.com/

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/20260806142321.3383353-1-sammiee5311@gmail.com?part=1

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

[HyeongJun An]

quickspi_hid_raw_request() receives the caller's buffer length in len, but
quickspi_get_report() never sees it and copies the whole device-supplied
response into buf regardless:

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

qsdev->report_len comes from the input report the touch controller returns,
while buf is sized to whatever the caller asked hidraw for through
HIDIOCGFEATURE or HIDIOCGINPUT.  A response larger than that overflows buf
with device-controlled content.

The intel-quicki2c sibling already passes the caller length down to
quicki2c_get_report() and validates the response against it before the
copy.  Do the same here.

Fixes: 4138f21115ae ("HID: intel-thc-hid: intel-quickspi: Complete THC QuickSPI driver")
Suggested-by: Sashiko AI <sashiko-bot@kernel.org>
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
v3: added READ_ONCE() around the qsdev->report_len read the v2 bound uses -
    quickspi_handle_input_data() writes this field from IRQ context with no
    locking, so per kernel convention for a lockless shared read it needs
    READ_ONCE() regardless of how many times the value is used afterward.
    Raised by the Sashiko AI review of v2.  No other change.

v2: fixed a Time-of-Check-to-Time-of-Use race the Sashiko AI review of v1
    pointed out - the new bound read qsdev->report_len once for the check
    and again for the memcpy, so a concurrent update between the two could
    have overflowed buf with the checked-against-a-smaller-value length.
    Read it once into a local and use that for both.

v1: https://lore.kernel.org/all/20260806133838.3378362-1-sammiee5311@gmail.com/

 .../intel-thc-hid/intel-quickspi/quickspi-hid.c  |  2 +-
 .../intel-quickspi/quickspi-protocol.c           | 16 +++++++++++++---
 .../intel-quickspi/quickspi-protocol.h           |  2 +-
 3 files changed, 15 insertions(+), 5 deletions(-)

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 91d5807b4a83..a60a0a7f16aa 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);
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 cb19057f1191..9dacfdf7aff6 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
@@ -342,10 +342,12 @@ int reset_tic(struct quickspi_device *qsdev)
 }
 
 int quickspi_get_report(struct quickspi_device *qsdev,
-			u8 report_type, unsigned int report_id, void *buf)
+			u8 report_type, unsigned int report_id, void *buf,
+			u32 buf_len)
 {
 	int rep_type;
 	int ret;
+	u32 report_len;
 
 	if (report_type == HID_INPUT_REPORT) {
 		rep_type = GET_INPUT_REPORT;
@@ -372,9 +374,17 @@ int quickspi_get_report(struct quickspi_device *qsdev,
 	}
 	qsdev->get_report_cmpl = false;
 
-	memcpy(buf, qsdev->report_buf, qsdev->report_len);
+	/* quickspi_handle_input_data() updates this from IRQ context. */
+	report_len = READ_ONCE(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);
 
-	return qsdev->report_len;
+	return report_len;
 }
 
 int quickspi_set_report(struct quickspi_device *qsdev,
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 775e29c1ed13..8a2338bee808 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);
 int quickspi_get_report(struct quickspi_device *qsdev, u8 report_type,
-			unsigned int report_id, void *buf);
+			unsigned int report_id, void *buf, u32 buf_len);
 int quickspi_set_report(struct quickspi_device *qsdev, u8 report_type,
 			unsigned int report_id, void *buf, u32 buf_len);
 int quickspi_get_report_descriptor(struct quickspi_device *qsdev);
-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Heap buffer overflow in `read_dma_buffer()` and `quickspi_handle_input_data()` due to mismatched bounds checking against the 4KB DMA limit.
- [Critical] Heap buffer overflow in `quickspi_set_report()` due to missing bounds check against the allocated size of `qsdev->report_buf`.
- [High] Data race and buffer corruption due to missing serialization across request and response handlers.
- [High] Use-After-Free in the IRQ thread handler against `qsdev->hid_dev` during device removal.
- [High] Missing CPU/Device synchronization for streaming DMA buffers.
- [Medium] State desynchronization in `quickspi_get_report()` and `quickspi_set_report()` due to stale completion flags.
--

--- Patch [1]: [PATCH v3] HID: intel-thc-hid: intel-quickspi: bound GET_REPORT response to the caller buffer ---
commit a1a09cc358271f5bac5710688c074796a3f0d94b
Author: HyeongJun An <sammiee5311@gmail.com>

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

This patch adds bounds checking in quickspi_get_report() to prevent the
device-supplied response length from overflowing the caller-provided buffer.
It ensures that the copied response length does not exceed the size requested
by the caller, mitigating potential memory corruption.

> 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 could this lead to a heap buffer overflow?

Userspace can invoke the HIDIOCSFEATURE or HIDIOCSOUTPUT ioctl via hidraw
with a payload up to HID_MAX_BUFFER_SIZE (8192 bytes). quickspi_set_report()
receives this user-controlled buf_len and passes it down:

quickspi-protocol.c:quickspi_set_report() {
    ...
    ret = write_cmd_to_txdma(qsdev, rep_type, report_id, buf + 1, buf_len - 1);
}

write_cmd_to_txdma() then performs a memcpy directly into qsdev->report_buf:

quickspi-protocol.c:write_cmd_to_txdma() {
    ...
    if (report_buf && report_buf_len > 0)
        memcpy(write_buf->content, report_buf, report_buf_len);
}

Does the driver ever verify that the user-supplied length fits within the
dynamically allocated capacity of qsdev->report_buf before copying?

>  		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..9dacfdf7aff64 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,17 @@ int quickspi_get_report(struct quickspi_device *qsdev,
>  	}
>  	qsdev->get_report_cmpl = false;

[Severity: Medium]
This is a pre-existing issue, but does this leave the completion flag
vulnerable to state desynchronization?

The driver waits for qsdev->get_report_cmpl using
wait_event_interruptible_timeout(). If the wait times out (e.g., due to
device latency or signal interruption), the function returns -ETIMEDOUT but
leaves the completion flag unmodified.

When the delayed response finally arrives, the IRQ handler will set the flag
to true.

Since write_cmd_to_txdma() is called earlier without first explicitly
re-initializing the completion flag to false:

quickspi-protocol.c:quickspi_get_report() {
    ...
    ret = write_cmd_to_txdma(qsdev, rep_type, report_id, NULL, 0);
}

Will the next request immediately pass the wait queue condition because the
flag is already true from the delayed response, leading to reading stale
data?

> -	memcpy(buf, qsdev->report_buf, qsdev->report_len);
> +	/* quickspi_handle_input_data() updates this from IRQ context. */
> +	report_len = READ_ONCE(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: High]
This is a pre-existing issue, but is there a data race and potential buffer
corruption due to missing serialization here?

The HID core does not universally serialize hid_hw_raw_request() calls,
so multiple userspace threads could concurrently invoke quickspi_get_report()
and quickspi_set_report().

write_cmd_to_txdma() locklessly writes outgoing commands directly into the
shared qsdev->report_buf:

quickspi-protocol.c:write_cmd_to_txdma() {
    ...
    write_buf = (struct output_report *)qsdev->report_buf;
    write_buf->output_hdr.report_type = report_type;
}

The threaded IRQ handler quickspi_handle_input_data() also locklessly writes
incoming responses into the exact same qsdev->report_buf:

quickspi-protocol.c:quickspi_handle_input_data() {
    ...
    memcpy(qsdev->report_buf, input_report, qsdev->report_len);
}

If two threads send requests simultaneously, or if an incoming response
arrives while another thread is reading the response here via memcpy(),
could their payloads interleave and corrupt outgoing DMA commands or
incoming responses?

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

[Severity: Critical]
This is a pre-existing issue, but is there a potential heap buffer overflow
in read_dma_buffer() and quickspi_handle_input_data()?

The driver allocates qsdev->input_buf and qsdev->report_buf based on
max_input_len and max_output_len from the device descriptor (e.g., 512
bytes). However, thc_dma_set_max_packet_sizes() aligns the max packet size
to 4096 bytes.

In read_dma_buffer(), it validates the packet size against this 4096-byte
limit rather than the allocated buffer size:

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 device sends a packet larger than the allocated max_input_len, could
sg_copy_to_buffer() overflow qsdev->input_buf?

Furthermore, quickspi_handle_input_data() performs a memcpy() into
qsdev->report_buf without validating against its allocated size:

quickspi-protocol.c:quickspi_handle_input_data() {
    ...
    memcpy(qsdev->report_buf, input_report, qsdev->report_len);
}

Can this lead to a secondary overflow if the device provides an untrusted
length?

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free in the
IRQ thread handler during device removal?

During removal, quickspi_remove() destroys the HID device while the threaded
IRQ handler is still registered and active:

pci-quickspi.c:quickspi_remove() {
    ...
    quickspi_hid_remove(qsdev);
    quickspi_dma_deinit(qsdev);
}

Since devres IRQ cleanup runs after remove() returns, and the interrupt is
not explicitly disabled before the HID device is destroyed, could an
interrupt fire concurrently?

If so, quickspi_irq_thread_handler() will run and eventually call
quickspi_hid_send_report(), which dereferences the freed HID device:

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

[Severity: High]
This is a pre-existing issue, but does the DMA code miss CPU/device
synchronization for streaming DMA buffers?

The driver uses dma_map_sg() to map its RX/TX ring buffers. In
read_dma_buffer(), it accesses the buffers via sg_copy_to_buffer() without
calling dma_sync_sg_for_cpu() to explicitly transfer ownership from the
device to the CPU:

intel-thc-dma.c:read_dma_buffer() {
    ...
    ret = sg_copy_to_buffer(sg, nent, read_buff, mes_len);
}

Similarly, in write_dma_buffer(), it writes data using sg_copy_from_buffer()
but never calls dma_sync_sg_for_device() before starting the DMA:

intel-thc-dma.c:write_dma_buffer() {
    ...
    ret = sg_copy_from_buffer(sg, nent, buffer, buf_len);
}

Can this lead to reading stale cache lines or device writing to un-bounced
memory on systems using SWIOTLB or non-coherent architectures?

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

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

[Xu, Even]

> -----Original Message-----
> From: HyeongJun An <sammiee5311@gmail.com>
> Sent: Thursday, August 6, 2026 10:56 PM
> To: Jiri Kosina <jikos@kernel.org>; Xu, Even <even.xu@intel.com>; Sun, Xinpeng
> <xinpeng.sun@intel.com>
> Cc: Benjamin Tissoires <bentiss@kernel.org>; linux-input@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org; HyeongJun An
> <sammiee5311@gmail.com>; Sashiko AI <sashiko-bot@kernel.org>
> Subject: [PATCH v3] HID: intel-thc-hid: intel-quickspi: bound GET_REPORT
> response to the caller buffer
> 
> quickspi_hid_raw_request() receives the caller's buffer length in len, but
> quickspi_get_report() never sees it and copies the whole device-supplied
> response into buf regardless:
> 
>     memcpy(buf, qsdev->report_buf, qsdev->report_len);
> 
> qsdev->report_len comes from the input report the touch controller
> qsdev->returns,
> while buf is sized to whatever the caller asked hidraw for through
> HIDIOCGFEATURE or HIDIOCGINPUT.  A response larger than that overflows buf
> with device-controlled content.
> 
> The intel-quicki2c sibling already passes the caller length down to
> quicki2c_get_report() and validates the response against it before the copy.  Do
> the same here.
> 
> Fixes: 4138f21115ae ("HID: intel-thc-hid: intel-quickspi: Complete THC QuickSPI
> driver")
> Suggested-by: Sashiko AI <sashiko-bot@kernel.org>
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
> ---
> v3: added READ_ONCE() around the qsdev->report_len read the v2 bound uses -
>     quickspi_handle_input_data() writes this field from IRQ context with no
>     locking, so per kernel convention for a lockless shared read it needs
>     READ_ONCE() regardless of how many times the value is used afterward.
>     Raised by the Sashiko AI review of v2.  No other change.
> 
> v2: fixed a Time-of-Check-to-Time-of-Use race the Sashiko AI review of v1
>     pointed out - the new bound read qsdev->report_len once for the check
>     and again for the memcpy, so a concurrent update between the two could
>     have overflowed buf with the checked-against-a-smaller-value length.
>     Read it once into a local and use that for both.
> 
> v1: https://lore.kernel.org/all/20260806133838.3378362-1-
> sammiee5311@gmail.com/
> 
>  .../intel-thc-hid/intel-quickspi/quickspi-hid.c  |  2 +-
>  .../intel-quickspi/quickspi-protocol.c           | 16 +++++++++++++---
>  .../intel-quickspi/quickspi-protocol.h           |  2 +-
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> 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 91d5807b4a83..a60a0a7f16aa 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); 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 cb19057f1191..9dacfdf7aff6 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> @@ -342,10 +342,12 @@ int reset_tic(struct quickspi_device *qsdev)  }
> 
>  int quickspi_get_report(struct quickspi_device *qsdev,
> -			u8 report_type, unsigned int report_id, void *buf)
> +			u8 report_type, unsigned int report_id, void *buf,
> +			u32 buf_len)
>  {
>  	int rep_type;
>  	int ret;
> +	u32 report_len;
> 
>  	if (report_type == HID_INPUT_REPORT) {
>  		rep_type = GET_INPUT_REPORT;
> @@ -372,9 +374,17 @@ int quickspi_get_report(struct quickspi_device *qsdev,
>  	}
>  	qsdev->get_report_cmpl = false;
> 
> -	memcpy(buf, qsdev->report_buf, qsdev->report_len);
> +	/* quickspi_handle_input_data() updates this from IRQ context. */
> +	report_len = READ_ONCE(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);
> 
> -	return qsdev->report_len;
> +	return report_len;
>  }
> 
>  int quickspi_set_report(struct quickspi_device *qsdev, 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 775e29c1ed13..8a2338bee808 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);
> int quickspi_get_report(struct quickspi_device *qsdev, u8 report_type,
> -			unsigned int report_id, void *buf);
> +			unsigned int report_id, void *buf, u32 buf_len);
>  int quickspi_set_report(struct quickspi_device *qsdev, u8 report_type,
>  			unsigned int report_id, void *buf, u32 buf_len);  int
> quickspi_get_report_descriptor(struct quickspi_device *qsdev);

Thanks for the patch!
This is a good error handling for unexpected big receive buffer.

Reviewed-by: Even Xu <even.xu@intel.com>

> --
> 2.43.0