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

[sashiko-bot@kernel.org]

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