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

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

[Dmitry Torokhov]

Hi Jinmo,

On Sun, May 31, 2026 at 12:59:30AM +0900, Jinmo Yang wrote:
> 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.
> 
> Use GFP_ATOMIC instead. The existing code already handles allocation
> failure by skipping the fifo entry and continuing.
> 
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

If you want to give credit this should be "Reported-by: Sashiko-bot <...>".

> Fixes: 5e013ad20689 ("HID: wacom: Remove static WACOM_PKGLEN_MAX limit")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
>  drivers/hid/wacom_sys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index a32320b35..2e237bdd2 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);
>  			continue;

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

As a followup please consider changing 'buf' management to use cleanup
facilities:

		u8 *buf __free(kfree) = kzalloc(size, GFP_ATOMIC);
		if (!buf) {
			kfifo_skiip(fifo);
			continue;
		}
		...

Thanks.

-- 
Dmitry