Re: [PATCH v2] HID: pidff: Fix integer overflow in pidff_rescale

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The subtraction `field->logical_maximum - field->logical_minimum` is evaluated in 32-bit arithmetic, allowing it to overflow before being promoted to `s64`.
- [High] The adjacent function `pidff_rescale_signed()` suffers from the exact same integer overflow vulnerability but was completely missed by the patch.
- [High] This problem wasn't introduced by this patch, but missing bounds check on `unit_exponent` in `pidff_rescale_time()` allows a kernel stall via a malicious HID descriptor.
--

commit d365813aa395ac1ce1eeffd147aa3beb65aba60b
Author: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>
HID: pidff: Fix integer overflow in pidff_rescale

This commit attempts to fix an integer overflow in pidff_rescale by using a
64-bit signed integer for calculations. It addresses an issue where rescaling
values close to the maximum could temporarily exceed the 32-bit signed
integer range.

Closes: https://github.com/JacKeTUs/universal-pidff/issues/116

> diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
> index aee8a44433059..c45f182d04480 100644
> --- a/drivers/hid/usbhid/hid-pidff.c
> +++ b/drivers/hid/usbhid/hid-pidff.c

[ ... ]

> @@ -326,8 +327,10 @@ static s32 pidff_clamp(s32 i, struct hid_field *field)
>   */
>  static int pidff_rescale(int i, int max, struct hid_field *field)
>  {
> -	return i * (field->logical_maximum - field->logical_minimum) / max +
> -	       field->logical_minimum;
> +	/* 64 bits needed for big values during rescale */
> +	s64 result = field->logical_maximum - field->logical_minimum;

Since both logical_maximum and logical_minimum are 32-bit signed integers,
does the subtraction field->logical_maximum - field->logical_minimum evaluate
in 32-bit arithmetic and potentially overflow before being promoted to s64?

If an untrusted USB device provides a descriptor with extreme limits,
this difference could exceed the capacity of a 32-bit signed integer and wrap
around to a negative value. Should at least one operand be explicitly cast
to s64 before the subtraction?

> +
> +	return div_s64(result * i, max) + field->logical_minimum;
>  }
>  
>  /*

This isn't a regression introduced by this patch, but looking at the
adjacent pidff_rescale_signed() function:

drivers/hid/usbhid/hid-pidff.c:pidff_rescale_signed() {
...
	if (i > 0)
		return i * field->logical_maximum / S16_MAX;
...
}

Does the multiplication i * field->logical_maximum suffer from a similar
overflow?

Since both operands fit into an int, they evaluate in 32-bit signed
arithmetic. If a device advertises a logical_maximum greater than 65535,
could this multiplication silently overflow yielding a wrapped-around
negative value before the division takes place?

Also not introduced by this patch, but looking at pidff_rescale_time():

drivers/hid/usbhid/hid-pidff.c:pidff_rescale_time() {
...
	int exponent = field->unit_exponent;

	pr_debug("time field exponent: %d\n", exponent);
	for (; exponent < FF_TIME_EXPONENT; exponent++)
		scaled_time *= 10;
	for (; exponent > FF_TIME_EXPONENT; exponent--)
		scaled_time /= 10;
...
}

Are there any bounds checks on field->unit_exponent before it is used
in these loops?

The field->unit_exponent is parsed from untrusted USB device descriptors.
If a USB device provides a large 32-bit exponent value, could
this execute up to 2.14 billion iterations doing multiplications or
divisions without any early break checks?

Could this trigger a kernel stall or soft lockup when a user uploads a
force-feedback effect?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510122352.1161826-1-tomasz.pakula.oficjalny@gmail.com?part=1