[PATCH] HID: ft260: fix stack-use-after-return write in I2C read race

[PATCH] HID: ft260: fix stack-use-after-return write in I2C read race

[Raman Varabets]

ft260_i2c_read() points dev->read_buf at a caller-supplied buffer
(often an on-stack variable), arms a completion and waits up to five
seconds for the device to return the data. The HID input callback
ft260_raw_event() runs in the input/IRQ path, independent of the
dev->lock mutex held by the read path, and copies the device-supplied
payload into dev->read_buf after a plain NULL check.

These two paths share read_buf, read_idx and read_len with no
serialization. If the device delays its response until the read
times out, ft260_i2c_read() resets the controller, clears read_buf
and returns, unwinding the stack frame the buffer lived in. A
response that arrives at that moment lets ft260_raw_event() pass the
NULL check and then memcpy() the device-controlled payload into the
now-freed stack location, a bounded but attacker-influenced
stack-use-after-return write triggerable by malicious or
malfunctioning hardware.

Add a dedicated spinlock that serializes every access to read_buf,
read_idx and read_len. ft260_raw_event() now holds it across the
NULL check, the memcpy and the index update, while the read path
takes it when arming and when clearing the buffer, so the teardown
can no longer slip between the check and the copy.

Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
Cc: stable@vger.kernel.org
Signed-off-by: Raman Varabets <kernel-linux-20260610-80b7ab08@raman.v1.sg>
---
 drivers/hid/hid-ft260.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 70e2eedb4..f47945954 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -240,6 +240,8 @@ struct ft260_device {
 	struct mutex lock;
 	u8 write_buf[FT260_REPORT_MAX_LENGTH];
 	unsigned long need_wakeup_at;
+	/* Protects read_buf, read_idx and read_len against ft260_raw_event() */
+	spinlock_t read_lock;
 	u8 *read_buf;
 	u16 read_idx;
 	u16 read_len;
@@ -501,6 +503,7 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 	int timeout, ret = 0;
 	struct ft260_i2c_read_request_report rep;
 	struct hid_device *hdev = dev->hdev;
+	unsigned long irqflags;
 	u8 bus_busy = 0;
 
 	if ((flag & FT260_FLAG_START_REPEATED) == FT260_FLAG_START_REPEATED)
@@ -526,9 +529,11 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 
 		reinit_completion(&dev->wait);
 
+		spin_lock_irqsave(&dev->read_lock, irqflags);
 		dev->read_idx = 0;
 		dev->read_buf = data;
 		dev->read_len = rd_len;
+		spin_unlock_irqrestore(&dev->read_lock, irqflags);
 
 		ret = ft260_hid_output_report(hdev, (u8 *)&rep, sizeof(rep));
 		if (ret < 0) {
@@ -543,7 +548,9 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 			goto ft260_i2c_read_exit;
 		}
 
+		spin_lock_irqsave(&dev->read_lock, irqflags);
 		dev->read_buf = NULL;
+		spin_unlock_irqrestore(&dev->read_lock, irqflags);
 
 		if (flag & FT260_FLAG_STOP)
 			bus_busy = FT260_I2C_STATUS_BUS_BUSY;
@@ -562,7 +569,9 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 	} while (len > 0);
 
 ft260_i2c_read_exit:
+	spin_lock_irqsave(&dev->read_lock, irqflags);
 	dev->read_buf = NULL;
+	spin_unlock_irqrestore(&dev->read_lock, irqflags);
 	return ret;
 }
 
@@ -1018,6 +1027,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 "FT260 usb-i2c bridge");
 
 	mutex_init(&dev->lock);
+	spin_lock_init(&dev->read_lock);
 	init_completion(&dev->wait);
 
 	ret = ft260_xfer_status(dev, FT260_I2C_STATUS_BUS_BUSY);
@@ -1067,6 +1077,7 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
 {
 	struct ft260_device *dev = hid_get_drvdata(hdev);
 	struct ft260_i2c_input_report *xfer = (void *)data;
+	unsigned long irqflags;
 
 	if (size < offsetof(struct ft260_i2c_input_report, data)) {
 		hid_err(hdev, "short report %d\n", size);
@@ -1075,6 +1086,8 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	if (xfer->report >= FT260_I2C_REPORT_MIN &&
 	    xfer->report <= FT260_I2C_REPORT_MAX) {
+		bool complete_read;
+
 		ft260_dbg("i2c resp: rep %#02x len %d size %d\n",
 			  xfer->report, xfer->length, size);
 
@@ -1085,8 +1098,15 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
 			return -1;
 		}
 
+		/*
+		 * Hold read_lock so a timed-out ft260_i2c_read() cannot
+		 * clear read_buf between the NULL check and the memcpy.
+		 */
+		spin_lock_irqsave(&dev->read_lock, irqflags);
+
 		if ((dev->read_buf == NULL) ||
 		    (xfer->length > dev->read_len - dev->read_idx)) {
+			spin_unlock_irqrestore(&dev->read_lock, irqflags);
 			hid_err(hdev, "unexpected report %#02x, length %d\n",
 				xfer->report, xfer->length);
 			return -1;
@@ -1095,8 +1115,11 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
 		memcpy(&dev->read_buf[dev->read_idx], &xfer->data,
 		       xfer->length);
 		dev->read_idx += xfer->length;
+		complete_read = dev->read_idx == dev->read_len;
+
+		spin_unlock_irqrestore(&dev->read_lock, irqflags);
 
-		if (dev->read_idx == dev->read_len)
+		if (complete_read)
 			complete(&dev->wait);
 
 	} else {
-- 
2.54.0

Re: [PATCH] HID: ft260: fix stack-use-after-return write in I2C read race

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver calls `hid_hw_open()` and `hid_hw_start()` before fully initializing driver data and locks, allowing an early interrupt to cause a NULL pointer dereference or uninitialized spinlock access.
- [High] Calling `complete(&dev->wait)` outside the `read_lock` creates a race condition where a delayed completion signal can spuriously wake up a subsequent I2C read, resulting in the exposure of uninitialized memory.
- [High] A delayed I2C response from a timed-out read can erroneously fulfill a newly issued I2C read request, silently corrupting the read payload.
--

commit f93912f569da4f226a3bdb7379cc9b9a98ba1eb9
Author: Raman Varabets <kernel-linux-20260610-80b7ab08@raman.v1.sg>

HID: ft260: fix stack-use-after-return write in I2C read race

This commit fixes a race condition where a delayed I2C response could write
into a freed stack buffer after a timeout in ft260_i2c_read. It adds a
dedicated spinlock to serialize accesses to the read buffer, index, and length
between the read path and the raw event handler.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 70e2eedb465af..f479459544aee 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
[ ... ]
> @@ -543,7 +548,9 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
>  			goto ft260_i2c_read_exit;
>  		}
>  
> +		spin_lock_irqsave(&dev->read_lock, irqflags);
>  		dev->read_buf = NULL;
> +		spin_unlock_irqrestore(&dev->read_lock, irqflags);

[Severity: High]
This is a pre-existing issue, but does the timeout path leave stale USB input
reports pending?

If an I2C read times out, ft260_i2c_read() resets the controller but does not
flush the HID/USB receive queues. When a new I2C read begins, dev->read_buf
is updated to point to the new buffer.

Could a delayed USB report containing the response for the previous timed-out
read arrive, pass the NULL check in ft260_raw_event(), and blindly copy stale
data into the new buffer? This would silently corrupt the payload and
desynchronize the bus state.

[ ... ]
> @@ -1018,6 +1027,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		 "FT260 usb-i2c bridge");
>  
>  	mutex_init(&dev->lock);
> +	spin_lock_init(&dev->read_lock);
>  	init_completion(&dev->wait);
>  
>  	ret = ft260_xfer_status(dev, FT260_I2C_STATUS_BUS_BUSY);

[Severity: High]
This isn't a bug introduced by this patch, but does hid_hw_open() allow events
to trigger before the driver data and locks are fully initialized?

In ft260_probe(), hid_hw_open() is called before hid_set_drvdata() and before
spin_lock_init(&dev->read_lock). Because hid_hw_open() submits USB interrupt
IN URBs, the hardware can start sending events immediately.

If ft260_raw_event() triggers early, hid_get_drvdata() would return NULL.
Could this lead to a NULL pointer dereference when ft260_raw_event() attempts
to acquire dev->read_lock?

[ ... ]
> @@ -1095,8 +1115,11 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
>  		memcpy(&dev->read_buf[dev->read_idx], &xfer->data,
>  		       xfer->length);
>  		dev->read_idx += xfer->length;
> +		complete_read = dev->read_idx == dev->read_len;
> +
> +		spin_unlock_irqrestore(&dev->read_lock, irqflags);
>  
> -		if (dev->read_idx == dev->read_len)
> +		if (complete_read)
>  			complete(&dev->wait);
>  
>  	} else {

[Severity: High]
This is a pre-existing issue, but does calling complete() outside the
read_lock create a race condition with the teardown path?

If an I2C read times out, ft260_i2c_read() wakes up and clears dev->read_buf.
A new I2C read request could then be issued, which calls
reinit_completion(&dev->wait).

If the softirq executing ft260_raw_event() is preempted between dropping the
lock and calling complete(&dev->wait), could it resume and signal the
completion after the new request has already reinitialized it? This would
cause the new wait to instantly succeed, returning success without writing any
data to the caller's buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610142952.3335586-1-kernel-linux-20260610-80b7ab08@raman.v1.sg?part=1