Re: [PATCH 1/1] HID: wacom: validate report size before kfifo insert

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Incomplete vulnerability fix / off-by-two error allowing kfifo state corruption and massive out-of-bounds slab read.

Pre-existing issues:
- [High] Sleeping in atomic context due to kzalloc(GFP_KERNEL) in wacom_wac_queue_flush().
--

commit 077dd88850a2e1126c613aafa226a5f028a98bb9
Author: Jinmo Yang <jinmo44.yang@gmail.com>

HID: wacom: validate report size before kfifo insert

This commit adds a check in wacom_wac_queue_insert() to reject reports
that exceed the total kfifo capacity. This aims to prevent oversized
reports from causing out-of-bounds writes during kfifo insertion.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index a32320b351e3e..cc82c6f36901a 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -54,6 +54,12 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
>  {
>  	bool warned = false;
>  
> +	if (size > kfifo_size(fifo)) {

[Severity: Critical]
Does this validation correctly account for the record header overhead?

Since the driver uses a record-based kfifo (kfifo_rec_ptr_2), each inserted 
record is prefixed with a 2-byte length header. This means the maximum 
available payload space in an empty kfifo is kfifo_size(fifo) - 2.

If an attacker sends a report with a size exactly equal to kfifo_size(fifo) 
or kfifo_size(fifo) - 1, this check falsely passes.

> +		hid_warn(hdev, "%s: report too large (%d > %u) for kfifo\n",
> +			 __func__, size, kfifo_size(fifo));
> +		return;
> +	}
> +
>  	while (kfifo_avail(fifo) < size) {

If the payload requires more space than the empty kfifo can provide, 
the kfifo_avail() check will be smaller than size, and the loop will 
execute on an empty buffer:

>  		if (!warned)
>  			hid_warn(hdev, "%s: kfifo has filled, starting to drop events\n", __func__);
>  		warned = true;
>  
>  		kfifo_skip(fifo);
>  	}

Could calling kfifo_skip() on an empty kfifo underflow the internal 
pointers and corrupt the ring buffer's state? 

If this allows the subsequent kfifo_in() to write an oversized payload 
and overwrite the 2-byte record header with attacker-controlled data, 
would this lead to a massive out-of-bounds read later when 
wacom_wac_queue_flush() processes the corrupted record?

[Severity: High]
This is a pre-existing issue, but does the related flush function 
wacom_wac_queue_flush() safely allocate memory?

It appears wacom_wac_queue_flush() calls kzalloc() with GFP_KERNEL. 
Since this queue processing path is invoked from wacom_raw_event() 
(which executes in an atomic context from the USB URB completion handler), 
could this GFP_KERNEL allocation sleep and trigger a 'scheduling while 
atomic' panic? Should this use GFP_ATOMIC instead?

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