[PATCH] HID: tmff: Use 64-bit arithmetic for force feedback scaling

[PATCH] HID: tmff: Use 64-bit arithmetic for force feedback scaling

[Linmao Li]

The logical minimum and maximum values come from the HID report
descriptor and cover the full signed 32-bit range.  Subtracting them in
an int can overflow before the force feedback value is scaled.  The
subsequent multiplication can overflow as well, producing an incorrect
value despite the final range checks.

Use 64-bit intermediates for both scaling helpers, as done by commit
48d1677779ad ("HID: pidff: Fix integer overflow in pidff_rescale") for
the same arithmetic in the PID driver.  This keeps the arithmetic
defined for the complete descriptor range before the result is clamped.

Fixes: dc76c912145f ("Input: use new FF interface in the HID force feedback drivers")
Fixes: b27c9590ca0f ("HID: add support for Thrustmaster FGT Force Feedback wheel")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/hid/hid-tmff.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-tmff.c b/drivers/hid/hid-tmff.c
index 423f395d01ac..319e7b670384 100644
--- a/drivers/hid/hid-tmff.c
+++ b/drivers/hid/hid-tmff.c
@@ -17,6 +17,7 @@
 
 #include <linux/hid.h>
 #include <linux/input.h>
+#include <linux/math64.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 
@@ -47,9 +48,9 @@ struct tmff_device {
 /* Changes values from 0 to 0xffff into values from minimum to maximum */
 static inline int tmff_scale_u16(unsigned int in, int minimum, int maximum)
 {
-	int ret;
+	s64 ret;
 
-	ret = (in * (maximum - minimum) / 0xffff) + minimum;
+	ret = div_s64((s64)in * ((s64)maximum - minimum), 0xffff) + minimum;
 	if (ret < minimum)
 		return minimum;
 	if (ret > maximum)
@@ -60,9 +61,9 @@ static inline int tmff_scale_u16(unsigned int in, int minimum, int maximum)
 /* Changes values from -0x80 to 0x7f into values from minimum to maximum */
 static inline int tmff_scale_s8(int in, int minimum, int maximum)
 {
-	int ret;
+	s64 ret;
 
-	ret = (((in + 0x80) * (maximum - minimum)) / 0xff) + minimum;
+	ret = div_s64((s64)(in + 0x80) * ((s64)maximum - minimum), 0xff) + minimum;
 	if (ret < minimum)
 		return minimum;
 	if (ret > maximum)
-- 
2.25.1

Re: [PATCH] HID: tmff: Use 64-bit arithmetic for force feedback scaling

[sashiko-bot]

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

Pre-existing issues:
- [High] Structural race condition leading to NULL pointer dereference in Force Feedback initialization.
--

--- Patch [1]: [PATCH] HID: tmff: Use 64-bit arithmetic for force feedback scaling ---
commit b7654294b005de01aa2d8e34c8846bfff883d73f
Author: Linmao Li <lilinmao@kylinos.cn>

    HID: tmff: Use 64-bit arithmetic for force feedback scaling

This commit fixes an integer overflow in force feedback scaling calculations
by using 64-bit intermediate variables. It updates both tmff_scale_u16() and
tmff_scale_s8() helpers to use div_s64() to prevent incorrect values when
processing logical minimum and maximums from HID report descriptors.

> diff --git a/drivers/hid/hid-tmff.c b/drivers/hid/hid-tmff.c
> index 423f395d01ac5..319e7b6703846 100644
> --- a/drivers/hid/hid-tmff.c
> +++ b/drivers/hid/hid-tmff.c
> @@ -17,6 +17,7 @@
>  
>  #include <linux/hid.h>
>  #include <linux/input.h>
> +#include <linux/math64.h>

[Severity: High]
This isn't a bug introduced by this patch, but does the force feedback
initialization in this driver have a structural race condition?

Looking at tm_probe(), hid_hw_start() fully registers the input device and
exposes the event node to userspace before tmff_init() configures force
feedback:

drivers/hid/hid-tmff.c:tm_probe() {
    ...
	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
	if (ret) {
		hid_err(hdev, "hw start failed\n");
		goto err;
	}

	tmff_init(hdev, (void *)id->driver_data);
    ...
}

Inside tmff_init(), capability bits are set and the FF structures are
initialized:

drivers/hid/hid-tmff.c:tmff_init() {
    ...
				for (i = 0; ff_bits[i] >= 0; i++)
					set_bit(ff_bits[i], input_dev->ffbit);

				break;
    ...
	error = input_ff_create_memless(input_dev, tmff, tmff_play);
    ...
}

If a local user monitors for device creation, they might immediately open the
event node and issue an EVIOCSFF ioctl.

This would execute input_ff_upload(), which checks that EV_FF and the effect
bits are set.  Since tmff_init() has already set the capabilities, could this
unconditionally call an uninitialized ff->upload pointer before
input_ff_create_memless() finishes setting it up?

Would it be possible to encounter a NULL pointer dereference here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710102314.190094-1-lilinmao@kylinos.cn?part=1