Re: [PATCH AUTOSEL 7.0-5.15] HID: ft260: validate i2c input report length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] memcpy reads beyond the bounds of the statically sized array 'data[2]', which will trigger a FORTIFY_SOURCE panic for payloads > 2 bytes.
- [High] A race condition in the probe function leads to a NULL pointer dereference in `ft260_raw_event` if an input report is received early.
- [High] A race condition exists between the HID raw event handler and the I2C read timeout path, leading to a Use-After-Free or kernel stack corruption.
- [High] The probe function incorrectly returns 0 (success) when intentionally rejecting the UART interface, rendering it unusable.
--

commit 4ff190a6df13fab19cc5a93ba01ca6de278d20bd
Author: Michael Zaidman <michael.zaidman@gmail.com>

HID: ft260: validate i2c input report length

This commit adds checks to ft260_raw_event to prevent out-of-bounds reads
from malicious or malfunctioning devices. It rejects reports shorter than the
2-byte header and validates the length field against the actual HID report
capacity.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 333341e80b0ec..70e2eedb465af 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -1068,10 +1068,22 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	struct ft260_device *dev = hid_get_drvdata(hdev);

[Severity: High]
This is a pre-existing issue, but can this result in a NULL pointer
dereference during device initialization?

If hid_hw_start() is called in ft260_probe() before hid_set_drvdata() is
initialized, the device might send an input report immediately (e.g., from
a pending interrupt). This would trigger ft260_raw_event(), leaving dev as
NULL and causing a panic when dev->read_buf is accessed below.

>  	struct ft260_i2c_input_report *xfer = (void *)data;
>  
> +	if (size < offsetof(struct ft260_i2c_input_report, data)) {
> +		hid_err(hdev, "short report %d\n", size);
> +		return -1;
> +	}
> +
>  	if (xfer->report >= FT260_I2C_REPORT_MIN &&
>  	    xfer->report <= FT260_I2C_REPORT_MAX) {
> -		ft260_dbg("i2c resp: rep %#02x len %d\n", xfer->report,
> -			  xfer->length);
> +		ft260_dbg("i2c resp: rep %#02x len %d size %d\n",
> +			  xfer->report, xfer->length, size);
> +
> +		if (xfer->length > size -
> +		    offsetof(struct ft260_i2c_input_report, data)) {
> +			hid_err(hdev, "report %#02x: length %d exceeds HID report size\n",
> +				xfer->report, xfer->length);
> +			return -1;
> +		}
>  
>  		if ((dev->read_buf == NULL) ||

[Severity: High]
This is a pre-existing issue, but does this check safely prevent a
use-after-free or stack corruption?

In ft260_i2c_read(), the driver blocks on wait_for_completion_timeout(). If a
timeout occurs, it sets dev->read_buf = NULL and returns, destroying the
caller-provided buffer which may be a stack frame.

If ft260_raw_event() checks dev->read_buf != NULL and is preempted while the
timeout path returns, wouldn't the subsequent memcpy() write hardware payload
data into freed memory or a popped stack frame?

>  		    (xfer->length > dev->read_len - dev->read_idx)) {


There is another pre-existing issue right below the diff in ft260_raw_event():

drivers/hid/hid-ft260.c:ft260_raw_event() {
	[ ... ]
		if ((dev->read_buf == NULL) ||
		    (xfer->length > dev->read_len - dev->read_idx)) {
			hid_err(hdev, "unexpected report %#02x, length %d\n",
				xfer->report, xfer->length);
			return -1;
		}

		memcpy(&dev->read_buf[dev->read_idx], &xfer->data,
		       xfer->length);
	[ ... ]
}

[Severity: High]
This isn't a bug introduced by this patch, but will this memcpy() trigger a
FORTIFY_SOURCE panic for payloads larger than 2 bytes?

The ft260_i2c_input_report structure defines the data payload as a fixed-size
array:

struct ft260_i2c_input_report {
	u8 report;
	u8 length;
	u8 data[2];
} __packed;

If xfer->length is greater than 2, the compiler statically knows the source
object size is exactly 2 bytes. Under CONFIG_FORTIFY_SOURCE=y, wouldn't this
trigger a mandatory kernel panic at runtime for a buffer overread? Could the
data field be changed to a flexible array member?


There is also a pre-existing issue in the probe function:

drivers/hid/hid-ft260.c:ft260_probe() {
	[ ... ]
	ret = ft260_is_interface_enabled(hdev);
	if (ret <= 0)
		goto err_hid_close;
	[ ... ]
err_hid_close:
	hid_hw_close(hdev);
err_hid_stop:
	hid_hw_stop(hdev);
	return ret;
}

[Severity: High]
This isn't a bug introduced by this patch, but does the probe function
incorrectly handle a rejected UART interface?

When ft260_is_interface_enabled() encounters the UART interface, it leaves
the return code as 0. The driver jumps to the error path, stops the hardware,
and returns 0 instead of an error code like -ENODEV.

Returning 0 informs the kernel driver core that hid-ft260 successfully bound
to the device interface. Doesn't this hijack the UART interface and leave it
in a disabled state, permanently preventing generic HID drivers (like
hid-generic) from claiming it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520111944.3424570-54-sashal@kernel.org?part=1