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

[Michael Zaidman]

Hi Raman,

Thanks for this patch. The race is real and separate from the input
report length checks already in for-7.1: those bound the HID report
source, but do not protect read_buf lifetime after a timed-out read
unwinds. A spinlock (not dev->lock) is the right fix for the IRQ/input
path.

For stable backport notes: did you reproduce the use-after-return, or
was this found by inspection?

Optional hardening for v2: initialize read_lock before hid_hw_open(), or
immediately after allocating dev, so a malicious USB device cannot hit
spin_lock on an uninitialized lock during the brief probe window after
the input path is live.

I'm validating on FT260 hardware and will follow up on this thread with
Tested-by when that is complete.

Reviewed-by: Michael Zaidman <michael.zaidman@xxxxxxxxx>


On Wed, Jun 10, 2026 at 5:30 PM Raman Varabets
<kernel-linux-20260610-80b7ab08@xxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Raman Varabets <kernel-linux-20260610-80b7ab08@xxxxxxxxxxx>
> ---
>  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
>