HID: hid-goodix-spi: out of bounds read in goodix_hid_irq()

[Maoyi Xie]

Hi all,

I think goodix_hid_irq() in drivers/hid/hid-goodix-spi.c can leak kernel
memory to a hidraw reader when a malicious touchscreen reports a large
package size. I would appreciate it if you could take a look.

The handler reads a coordinate event over SPI. It then passes a device
controlled length straight to hid_input_report().

	if (le16_to_cpu(pkg->size) < GOODIX_HID_PKG_LEN_SIZE) {
		dev_err(ts->dev, "invalid coordinate event package size, %d",
			le16_to_cpu(pkg->size));
		return IRQ_HANDLED;
	}
	hid_input_report(ts->hid, HID_INPUT_REPORT, pkg->data,
			 le16_to_cpu(pkg->size) - GOODIX_HID_PKG_LEN_SIZE, 1);

pkg->size is a __le16 from the device. The only check is a lower bound.
There is no upper bound. So pkg->size can be up to 65535, while pkg->data
only holds GOODIX_HID_COOR_DATA_LEN (82) bytes read into event_buf.

The length reaches hid_input_report(), which passes it as both the buffer
size and the report size.

	return __hid_input_report(hid, type, data, size, size, interrupt, ...);

Those two are equal, so the bsize < csize guard in hid_report_raw_event()
never fires. The device length then flows into the hidraw path.

	if (hid->claimed & HID_CLAIMED_HIDRAW)
		ret = hidraw_report_event(hid, data, size);

hidraw_report_event() does kmemdup(data, size) for each open reader. That
copies up to about 65 KB starting at pkg->data, far past the 82 valid bytes.
The bytes land in the buffer a hidraw client reads back. So it is an out of
bounds read of kernel slab memory handed to user space.

This needs a malicious or compromised Goodix SPI touchscreen and an open
/dev/hidraw node. The open node is common, since libinput and similar tools
keep one. It is not reachable from a VM or syzkaller, only from the SPI
device.

I reproduced the read on 7.1-rc7. I registered a HID device, opened a hidraw
reader, and called hid_input_report() with a small buffer and a large size.
The kmemdup copy faults.

  BUG: unable to handle page fault for address ... (in memcpy_orig)
  Call Trace:
   kmemdup_noprof
   hidraw_report_event
   hid_report_raw_event
   __hid_input_report

An upper bound on pkg->size before the call would close it. The driver
already has the lower bound. A matching upper bound against the bytes read
into event_buf would fit that check. The second hid_input_report() call
further down has the same shape.

Does this look like a real bug to you, and is an upper bound on pkg->size
the right fix? If so I am happy to send a proper patch with a Fixes tag and
Cc stable.

Thanks,
Maoyi
https://maoyixie.com/