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

[Tomasz Pakuła]

On Mon, 2026-11-02 at 23:23 +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.
> 
> Also warn during initialization if the duration field's unit exponent
> is outside the range -8 to 0. This helps identify unusual device
> descriptors without logging on every effect upload or changing the
> rescaling result.
> 
> Fixes: 8713107221a8 ("HID: pidff: Rescale time values to match field units")
> Reviewed-by: Tomasz Pakuła <tomasz.pakula.oficjalny@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Shengzhuo Wei <me@xxxxxxxx>

Afaik, the tags should be in a chronological order (my review on the
bottom) + reviews should be removed if patch contents have been changed.
My review is no longer viable for this v2

> ---
> v1: https://lore.kernel.org/r/20261029-b4-pidff-rescale-exponent-v1-1-3ba54accf767@xxxxxxxx
> 
> Changes in v2:
> - Following Tomasz's suggestion, warn during initialization when
>   the duration field's unit exponent is outside -8..0.
> - Add Tomasz's Reviewed-by.
> ---
>  drivers/hid/usbhid/hid-pidff.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
> index 22951b7ecd17..4540f49fe6e3 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);
> @@ -1428,11 +1428,19 @@ static int pidff_find_effects(struct pidff_device *pidff,
>   */
>  static int pidff_init_fields(struct pidff_device *pidff, struct input_dev *dev)
>  {
> +	int exp;

This driver no longer enforces C90 standard (in line with kernel) so
please, declare the variable where it's needed.

> +
>  	if (PIDFF_FIND_FIELDS(set_effect, PID_SET_EFFECT, 1)) {
>  		hid_err(pidff->hid, "unknown set_effect report layout\n");
>  		return -ENODEV;
>  	}
>  
> +	exp = pidff->set_effect[PID_DURATION].field->unit_exponent;

So here, int exp = ...

> +	if (exp < -8 || exp > 0)
> +		hid_warn(pidff->hid,
> +			 "duration field has unusual time unit exponent %d\n",
> +			 exp);
> +
>  	PIDFF_FIND_FIELDS(block_load, PID_BLOCK_LOAD, 0);
>  	if (!pidff->block_load[PID_EFFECT_BLOCK_INDEX].value) {
>  		hid_err(pidff->hid, "unknown pid_block_load report layout\n");
> 
> ---
> base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
> change-id: 20261028-b4-pidff-rescale-exponent-1d5c4e218e6d
> 
> Best regards,
> --  
> Shengzhuo Wei <me@xxxxxxxx>