Re: [PATCH] HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()

Re: [PATCH] HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] A malicious USB device can trigger a massive heap buffer overflow by sending a HID report larger than the wacom pen_fifo capacity (e.g., > PAGE_SIZE).
--

commit 2421694466917c461a3ca5695c18542757de2d44
Author: Jinmo Yang <jinmo44.yang@gmail.com>

HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()

wacom_wac_queue_flush() is called via the .raw_event callback
(wacom_raw_event → wacom_wac_pen_serial_enforce → wacom_wac_queue_flush).
For USB HID devices, this callback is invoked from hid_irq_in(), which
is a URB completion handler running in atomic context. Using GFP_KERNEL
in this path can sleep, leading to a "scheduling while atomic" bug.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2220168bf1164..a25400b3e658d 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -74,7 +74,7 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
>  		unsigned int count;
>  		int err;
>  
> -		buf = kzalloc(size, GFP_KERNEL);
> +		buf = kzalloc(size, GFP_ATOMIC);
>  		if (!buf) {
>  			kfifo_skip(fifo);

[Severity: Critical]
This is a pre-existing issue, but does this usage of kfifo_skip() point to a
similar problem in wacom_wac_queue_insert() in drivers/hid/wacom_sys.c?

If a malicious device spoofs a Wacom device with WACOM_QUIRK_TOOLSERIAL and
sends a report larger than PAGE_SIZE, wouldn't wacom_wac_queue_insert() be
called with size > PAGE_SIZE?

Since pen_fifo capacity is bounded to at most PAGE_SIZE, kfifo_avail(fifo) can
never naturally be >= size, causing the code to loop and blindly call
kfifo_skip(fifo):

drivers/hid/wacom_sys.c:wacom_wac_queue_insert() {
...
	while (kfifo_avail(fifo) < size) {
		if (!warned)
			hid_warn(hdev, "%s: kfifo has filled, starting to drop events\n", __func__);
		warned = true;

		kfifo_skip(fifo);
	}

	kfifo_in(fifo, raw_data, size);
...

Doesn't kfifo_skip(fifo) advance fifo->out even when empty, underflowing
fifo->in - fifo->out to a massive unsigned value?

Could this trick kfifo_avail(fifo) into returning a huge number, breaking the
loop and allowing kfifo_in() to write up to 16KB out of bounds into the 4KB
fifo->data buffer via lib/kfifo.c:kfifo_copy_in()?

lib/kfifo.c:kfifo_copy_in() {
...
	memcpy(fifo->data, src + l, len - l);
...

>  			continue;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530155930.128183-1-jinmo44.yang@gmail.com?part=1