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

[PATCH v2 0/2] HID: wacom: fix sleeping in atomic context in wacom_wac_queue_flush()

[Jinmo Yang]

wacom_wac_queue_flush() uses GFP_KERNEL for kzalloc, but it can be
called from atomic context via the .raw_event callback path. Patch 1
fixes this by switching to GFP_ATOMIC, and patch 2 converts the buffer
management to use __free(kfree) cleanup as suggested by Dmitry.

Changes since v1:
- Replaced Suggested-by with Reported-by for Sashiko-bot
- Added patch 2 to use __free(kfree) cleanup facility (Dmitry)

Jinmo Yang (2):
  HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()
  HID: wacom: use cleanup.h for wacom_wac_queue_flush() buffer
    management

 drivers/hid/wacom_sys.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()

[Jinmo Yang]

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.

Reported-by: Sashiko-bot <sashiko-bot@kernel.org>
Fixes: 5e013ad20689 ("HID: wacom: Remove static WACOM_PKGLEN_MAX limit")
Cc: stable@vger.kernel.org
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
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;
-- 
2.53.0

[PATCH v2 2/2] HID: wacom: use cleanup.h for wacom_wac_queue_flush() buffer management

[Jinmo Yang]

Use __free(kfree) cleanup facility for the temporary buffer in
wacom_wac_queue_flush() to simplify error paths and ensure the buffer
is freed automatically when it goes out of scope.

Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
---
 drivers/hid/wacom_sys.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2e237bdd2..edc24fe2e 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -70,11 +70,10 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
 {
 	while (!kfifo_is_empty(fifo)) {
 		int size = kfifo_peek_len(fifo);
-		u8 *buf;
+		u8 *buf __free(kfree) = kzalloc(size, GFP_ATOMIC);
 		unsigned int count;
 		int err;
 
-		buf = kzalloc(size, GFP_ATOMIC);
 		if (!buf) {
 			kfifo_skip(fifo);
 			continue;
@@ -87,7 +86,6 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
 			// to flush seems reasonable enough, however.
 			hid_warn(hdev, "%s: removed fifo entry with unexpected size\n",
 				 __func__);
-			kfree(buf);
 			continue;
 		}
 		err = hid_report_raw_event(hdev, HID_INPUT_REPORT, buf, size, size, false);
@@ -95,8 +93,6 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
 			hid_warn(hdev, "%s: unable to flush event due to error %d\n",
 				 __func__, err);
 		}
-
-		kfree(buf);
 	}
 }
 
-- 
2.53.0