[PATCH] HID: pidff: Use correct effect type in effect update

[PATCH] HID: pidff: Use correct effect type in effect update

[Oleg Makarenko]

When updating an existing effect, the effect type from the last created
effect was sent to the device instead of the updated one.
This caused incorrect reports when a game creates multiple different
effects and updates only one that is not the last created.

Fixes FFB in multiple games that create multiple simultaneous effects
(Forza Horizon 5/6).

Fixes: 224ee88fe395 ("Input: add force feedback driver for PID devices")
Cc: <stable@vger.kernel.org>
Tested-by: Oliver Roundtree <oroundtree1@gmail.com>
Co-developed-by: Ryno Kotzé <lemon.xah@gmail.com>
Signed-off-by: Ryno Kotzé <lemon.xah@gmail.com>
Signed-off-by: Oleg Makarenko <oleg@makarenk.ooo>
---
 drivers/hid/usbhid/hid-pidff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index c45f182d0448..5f4395f7c645 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -522,7 +522,7 @@ static void pidff_set_effect_report(struct pidff_device *pidff,
 	pidff->set_effect[PID_EFFECT_BLOCK_INDEX].value[0] =
 		pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0];
 	pidff->set_effect_type->value[0] =
-		pidff->create_new_effect_type->value[0];
+		pidff_get_effect_type_id(pidff, effect);
 
 	pidff_set_duration(&pidff->set_effect[PID_DURATION],
 			   effect->replay.length);
-- 
2.54.0

Re: [PATCH] HID: pidff: Use correct effect type in effect update

[sashiko-bot]

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

Pre-existing issues:
- [High] Pre-existing out-of-bounds read in `pidff_find_effects` leading to potential kernel panic (local DoS via crafted USB device).
--

commit 07c3b678f1bae4a1c16dfff6971345dc4b180f89
Author: Oleg Makarenko <oleg@makarenk.ooo>

HID: pidff: Use correct effect type in effect update

This commit fixes an issue where the effect type from the last created effect
was incorrectly used when updating existing effects. It ensures the correct
effect type ID is sent to the device, fixing force feedback in games that
manage multiple simultaneous effects.

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

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking at pidff_find_effects()
in this same file:

drivers/hid/usbhid/hid-pidff.c:pidff_find_effects() {
...
	for (i = 0; i < ARRAY_SIZE(pidff_effect_types); i++) {
		int pidff_type = pidff->type_id[i];

		if (pidff->set_effect_type->usage[pidff_type].hid !=
		    pidff->create_new_effect_type->usage[pidff_type].hid) {
			hid_err(pidff->hid,
				"effect type number %d is invalid\n", i);
			return -1;
		}
	}
...
}

Could this code cause an out-of-bounds read if a malicious USB device is
plugged in?

The type_id array is populated with 1-based logical indices, but pidff_type is
used directly as a 0-based index to access the usage arrays.

If the device descriptor defines many usages in create_new_effect_type but
very few in set_effect_type, would pidff_type exceed
set_effect_type->maxusage and read past the end of the array?

Does this risk a kernel panic if the read crosses page boundaries during
device probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609160031.493353-1-oleg@makarenk.ooo?part=1