[PATCH] Input: goodix - clamp the device-reported contact count

[PATCH] Input: goodix - clamp the device-reported contact count

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

goodix_ts_read_input_report() copies the number of touch points reported
by the device into an on-stack buffer

	u8 point_data[2 + GOODIX_MAX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];

which is sized for at most GOODIX_MAX_CONTACTS (10) contacts. The only
runtime check bounds the per-interrupt count against ts->max_touch_num,
but that value is taken verbatim from a 4-bit field of the device
configuration block and is never clamped:

	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;

The nibble can be 0..15, so a malfunctioning, malicious or counterfeit
controller (or an attacker tampering with the I2C bus) can advertise up
to 15 contacts. goodix_ts_read_input_report() then accepts a touch_num
of up to 15 and the second goodix_i2c_read() writes
ts->contact_size * (touch_num - 1) bytes past the one-contact header into
point_data - up to 30 bytes (45 with the 9-byte report format) beyond the
92-byte buffer: a stack out-of-bounds write.

Clamp max_touch_num to GOODIX_MAX_CONTACTS, the number of contacts
point_data[] is sized for, when reading it from the configuration.

Fixes: a7ac7c95d468 ("Input: goodix - use max touch number from device config")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 drivers/input/touchscreen/goodix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f8798d11ec03..17fcfe45988c 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1057,7 +1057,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
 	}
 
 	ts->int_trigger_type = ts->config[TRIGGER_LOC] & 0x03;
-	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
+	ts->max_touch_num = min(ts->config[MAX_CONTACTS_LOC] & 0x0f,
+				GOODIX_MAX_CONTACTS);
 
 	x_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC]);
 	y_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC + 2]);

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260612-b4-disp-6844625d-463f81173dc6

Best regards,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

Re: [PATCH] Input: goodix - clamp the device-reported contact count

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbalanced IRQ teardown and request on suspend/resume leads to kernel panic.
--

commit 67a6ac06922d6e5f9f42f13db85b8ae1bd396b75
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: goodix - clamp the device-reported contact count

This commit fixes an out-of-bounds stack write when reading the input
report. It clamps the maximum number of touch points reported by the
device configuration to prevent a malicious or malfunctioning controller
from overflowing the on-stack buffer.

> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index f8798d11ec030..17fcfe45988c2 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c

[Severity: High]
This isn't a bug introduced by this patch, but looking at the driver's
suspend and resume paths, is there a potential regression regarding null
pointer dereferences if device initialization fails asynchronously?

If asynchronous firmware loading or configuration fails, device
initialization aborts without unbinding the driver. The ts->input_dev
pointer remains uninitialized, and the interrupt is never requested.

During system suspend, goodix_suspend() unconditionally calls
goodix_free_irq(), which invokes devm_free_irq() without checking if the
interrupt was actually requested:

goodix_suspend()->goodix_free_irq():
    if (ts->client->irq)
        devm_free_irq(&ts->client->dev, ts->client->irq, ts);

This triggers a warning because the managed resource does not exist.

Furthermore, upon system resume, goodix_resume() unconditionally
calls goodix_request_irq():

goodix_resume():
    ...
    error = goodix_request_irq(ts);
    if (error)
        return error;

This successfully registers the interrupt handler even though the device
is completely uninitialized.

When a subsequent touch event triggers the interrupt,
goodix_process_events() blindly accesses the uninitialized input device:

goodix_process_events():
    ...
    input_mt_sync_frame(ts->input_dev);

Because ts->input_dev is uninitialized, does this dereference cause a
regression during device operation?

Could the suspend and resume functions be updated to verify the device is
fully initialized before tearing down and requesting the interrupt?

> @@ -1057,7 +1057,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
>  	}
>  
>  	ts->int_trigger_type = ts->config[TRIGGER_LOC] & 0x03;
> -	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
> +	ts->max_touch_num = min(ts->config[MAX_CONTACTS_LOC] & 0x0f,
> +				GOODIX_MAX_CONTACTS);
>  
>  	x_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC]);
>  	y_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC + 2]);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-b4-disp-6844625d-v1-1-df0aed080c9d@proton.me?part=1

Re: [PATCH] Input: goodix - clamp the device-reported contact count

[Hans de Goede]

Hi,

On 13-Jun-26 04:10, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> goodix_ts_read_input_report() copies the number of touch points reported
> by the device into an on-stack buffer
> 
> 	u8 point_data[2 + GOODIX_MAX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
> 
> which is sized for at most GOODIX_MAX_CONTACTS (10) contacts. The only
> runtime check bounds the per-interrupt count against ts->max_touch_num,
> but that value is taken verbatim from a 4-bit field of the device
> configuration block and is never clamped:
> 
> 	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
> 
> The nibble can be 0..15, so a malfunctioning, malicious or counterfeit
> controller (or an attacker tampering with the I2C bus) can advertise up
> to 15 contacts. goodix_ts_read_input_report() then accepts a touch_num
> of up to 15 and the second goodix_i2c_read() writes
> ts->contact_size * (touch_num - 1) bytes past the one-contact header into
> point_data - up to 30 bytes (45 with the 9-byte report format) beyond the
> 92-byte buffer: a stack out-of-bounds write.
> 
> Clamp max_touch_num to GOODIX_MAX_CONTACTS, the number of contacts
> point_data[] is sized for, when reading it from the configuration.
> 
> Fixes: a7ac7c95d468 ("Input: goodix - use max touch number from device config")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans


> ---
>  drivers/input/touchscreen/goodix.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index f8798d11ec03..17fcfe45988c 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1057,7 +1057,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
>  	}
>  
>  	ts->int_trigger_type = ts->config[TRIGGER_LOC] & 0x03;
> -	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
> +	ts->max_touch_num = min(ts->config[MAX_CONTACTS_LOC] & 0x0f,
> +				GOODIX_MAX_CONTACTS);
>  
>  	x_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC]);
>  	y_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC + 2]);
> 
> ---
> base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
> change-id: 20260612-b4-disp-6844625d-463f81173dc6
> 
> Best regards,

Re: [PATCH] Input: goodix - clamp the device-reported contact count

[Dmitry Torokhov]

On Sun, Jun 14, 2026 at 01:44:07PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 13-Jun-26 04:10, Bryam Vargas via B4 Relay wrote:
> > From: Bryam Vargas <hexlabsecurity@proton.me>
> > 
> > goodix_ts_read_input_report() copies the number of touch points reported
> > by the device into an on-stack buffer
> > 
> > 	u8 point_data[2 + GOODIX_MAX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
> > 
> > which is sized for at most GOODIX_MAX_CONTACTS (10) contacts. The only
> > runtime check bounds the per-interrupt count against ts->max_touch_num,
> > but that value is taken verbatim from a 4-bit field of the device
> > configuration block and is never clamped:
> > 
> > 	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
> > 
> > The nibble can be 0..15, so a malfunctioning, malicious or counterfeit
> > controller (or an attacker tampering with the I2C bus) can advertise up
> > to 15 contacts. goodix_ts_read_input_report() then accepts a touch_num
> > of up to 15 and the second goodix_i2c_read() writes
> > ts->contact_size * (touch_num - 1) bytes past the one-contact header into
> > point_data - up to 30 bytes (45 with the 9-byte report format) beyond the
> > 92-byte buffer: a stack out-of-bounds write.
> > 
> > Clamp max_touch_num to GOODIX_MAX_CONTACTS, the number of contacts
> > point_data[] is sized for, when reading it from the configuration.
> > 
> > Fixes: a7ac7c95d468 ("Input: goodix - use max touch number from device config")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> 
> Regards,
> 
> Hans
> 
> 
> > ---
> >  drivers/input/touchscreen/goodix.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index f8798d11ec03..17fcfe45988c 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -1057,7 +1057,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
> >  	}
> >  
> >  	ts->int_trigger_type = ts->config[TRIGGER_LOC] & 0x03;
> > -	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
> > +	ts->max_touch_num = min(ts->config[MAX_CONTACTS_LOC] & 0x0f,
> > +				GOODIX_MAX_CONTACTS);

Should we drop the report if is has bogus data in it?

Thanks.

-- 
Dmitry