[PATCH] HID: pidff: bound the unit exponent rescaling loop

[PATCH] HID: pidff: bound the unit exponent rescaling loop

[Shengzhuo Wei]

pidff_rescale_time() walks the device-provided unit exponent one step
at a time, allowing extreme values to cause billions of iterations
while ff->mutex is held. In a UHID test with an INT_MIN exponent,
EVIOCSFF took 3.1 to 4.3 seconds to upload a finite-duration effect.

Stop rescaling once scaled_time becomes zero, since further
multiplication or division by 10 cannot change the result. The u32
value becomes zero within 32 multiplications by 10, and the u16
input within 5 divisions. This bounds the loops while preserving
their results for all exponents, including wraparound.

Fixes: 8713107221a8 ("HID: pidff: Rescale time values to match field units")
Cc: stable@vger.kernel.org
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 drivers/hid/usbhid/hid-pidff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index 22951b7ecd17..1a2fffcf204e 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -354,9 +354,9 @@ static u32 pidff_rescale_time(u16 time, struct hid_field *field)
 	int exponent = field->unit_exponent;
 
 	pr_debug("time field exponent: %d\n", exponent);
-	for (; exponent < FF_TIME_EXPONENT; exponent++)
+	for (; scaled_time && exponent < FF_TIME_EXPONENT; exponent++)
 		scaled_time *= 10;
-	for (; exponent > FF_TIME_EXPONENT; exponent--)
+	for (; scaled_time && exponent > FF_TIME_EXPONENT; exponent--)
 		scaled_time /= 10;
 
 	pr_debug("time calculated from %d to %d\n", time, scaled_time);

---
base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
change-id: 20261028-b4-pidff-rescale-exponent-1d5c4e218e6d

Best regards,
--  
Shengzhuo Wei <me@cherr.cc>

Re: [PATCH] HID: pidff: bound the unit exponent rescaling loop

[Tomasz Pakuła]

On Thu, 2026-10-29 at 00:04 +0800, Shengzhuo Wei wrote:
> pidff_rescale_time() walks the device-provided unit exponent one step
> at a time, allowing extreme values to cause billions of iterations
> while ff->mutex is held. In a UHID test with an INT_MIN exponent,
> EVIOCSFF took 3.1 to 4.3 seconds to upload a finite-duration effect.
> 
> Stop rescaling once scaled_time becomes zero, since further
> multiplication or division by 10 cannot change the result. The u32
> value becomes zero within 32 multiplications by 10, and the u16
> input within 5 divisions. This bounds the loops while preserving
> their results for all exponents, including wraparound.
> 
> Fixes: 8713107221a8 ("HID: pidff: Rescale time values to match field units")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shengzhuo Wei <me@cherr.cc>
> ---
>  drivers/hid/usbhid/hid-pidff.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
> index 22951b7ecd17..1a2fffcf204e 100644
> --- a/drivers/hid/usbhid/hid-pidff.c
> +++ b/drivers/hid/usbhid/hid-pidff.c
> @@ -354,9 +354,9 @@ static u32 pidff_rescale_time(u16 time, struct hid_field *field)
>  	int exponent = field->unit_exponent;
>  
>  	pr_debug("time field exponent: %d\n", exponent);
> -	for (; exponent < FF_TIME_EXPONENT; exponent++)
> +	for (; scaled_time && exponent < FF_TIME_EXPONENT; exponent++)
>  		scaled_time *= 10;
> -	for (; exponent > FF_TIME_EXPONENT; exponent--)
> +	for (; scaled_time && exponent > FF_TIME_EXPONENT; exponent--)
>  		scaled_time /= 10;
>  
>  	pr_debug("time calculated from %d to %d\n", time, scaled_time);
> 
> ---
> base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
> change-id: 20261028-b4-pidff-rescale-exponent-1d5c4e218e6d
> 
> Best regards,
> --  
> Shengzhuo Wei <me@cherr.cc>

Tested this locally and works nice.

Should we maybe add a warning to the driver that the time exponent might
be out of a sane range? Sane range being, say, 10 to 0? This of course
would have to happen during init stage not to spam dmesg.

Regardless of that:
-- 
Reviewed-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>

Re: [PATCH] HID: pidff: bound the unit exponent rescaling loop

[Shengzhuo Wei]

Hi Tomasz,

Thanks for testing and reviewing the patch.

> Should we maybe add a warning to the driver that the time exponent might
> be out of a sane range? Sane range being, say, 10 to 0? This of course
> would have to happen during init stage not to spam dmesg.

For the initialization-time warning, would -8 to 0 be a reasonable
exponent range?

- -8 is the lowest exponent in the commonly used signed 4-bit HID
  encoding.
- 0 allows a time unit of one second.
- Positive exponents 1 through 7 would give time units of ten seconds
  or more, which seem unusually coarse for force-feedback effects.

This would only be a warning threshold: the driver would still accept
the device and leave the rescaling behavior unchanged.

Does that match what you had in mind, or would you prefer a different
range?

Thanks,
Shengzhuo

Re: [PATCH] HID: pidff: bound the unit exponent rescaling loop

[Tomasz Pakuła]

On Mon, 2026-11-02 at 22:40 +0800, Shengzhuo Wei wrote:
> Hi Tomasz,
> 
> Thanks for testing and reviewing the patch.
> 
> > Should we maybe add a warning to the driver that the time exponent might
> > be out of a sane range? Sane range being, say, 10 to 0? This of course
> > would have to happen during init stage not to spam dmesg.
> 
> For the initialization-time warning, would -8 to 0 be a reasonable
> exponent range?

Yes, I meant to say -10 (not positive 10 obviously) but -8 seems like a
better fit here.

> - -8 is the lowest exponent in the commonly used signed 4-bit HID
>   encoding.
> - 0 allows a time unit of one second.
> - Positive exponents 1 through 7 would give time units of ten seconds
>   or more, which seem unusually coarse for force-feedback effects.
> 
> This would only be a warning threshold: the driver would still accept
> the device and leave the rescaling behavior unchanged.
> 
> Does that match what you had in mind, or would you prefer a different
> range?

Yup, that range is great.

> 
> Thanks,
> Shengzhuo