Re: [PATCH RESEND 2] HID: Add force feedback support for Speedlink Cougar Vibration Flightstick

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Race condition in force feedback initialization leading to a NULL pointer dereference.
- [High] Ignoring the return value of `gembird_joy_init()` causes a NULL pointer dereference when user-space interacts with the event device after an initialization failure.
- [High] Non-monotonic magnitude scaling logic severely breaks force feedback functionality.
- [Medium] Memory leak of the `gembird_joy_device` state structure upon device disconnect.
--

commit f9141496ab06cd22000d2487bbb234d7e91931b5
Author: Harald Judt <h.judt@gmx.at>

HID: Add force feedback support for Gembird based joystick

This commit adds force feedback (rumble) support for the SpeedLink Cougar
Vibration Flightstick (SL-6630) joystick, which uses a Gembird controller.

> diff --git a/drivers/hid/hid-gembird-joy.c b/drivers/hid/hid-gembird-joy.c
> new file mode 100644
> index 0000000000000..5a5afa02f8407
> --- /dev/null
> +++ b/drivers/hid/hid-gembird-joy.c

[ ... ]

> +static int hid_gembird_joy_play(struct input_dev *dev, void *data,
> +				struct ff_effect *effect)
> +{
> +	struct hid_device *hid = input_get_drvdata(dev);
> +	struct gembird_joy_device *joy = data;
> +	int strong, weak;
> +
> +	strong = effect->u.rumble.strong_magnitude;
> +	weak = effect->u.rumble.weak_magnitude;

[ ... ]

> +	strong = (strong / 0xff) * 0x1f / 0xff;
> +	weak = (weak / 0xff) * 0x1f / 0xff;
> +
> +	/* ... and to support the notions of strong vs weak rumble effects,
> +	 * increase the magnitude for the strong rumble effect if it is below the
> +	 * half of the maximum value, as the strong motor has the same strength as
> +	 * the weak one. Likewise, decrease the magnitude for the weak effect.
> +	 */
> +	if (strong < 0x10 && !weak)         /* fftest effect 4 strong rumble */
> +		strong *= 2;
> +	else if (!strong && weak >= 0x10)   /* fftest effect 5 weak rumble */
> +		weak /= 2;

Will this cause issues with non-monotonic force feedback behavior and
unintended truncation?

Since the math uses integer division, any input value below 255 for strong
or weak will evaluate to zero during the first division by 0xff.

Additionally, could the conditional scaling cause abrupt changes in rumble
strength? For instance, if a game requests a strong magnitude that scales
down to 15, it gets doubled to 30 because of the strong < 0x10 check.
However, a slightly stronger requested magnitude that scales to 16 will
remain 16, meaning an increase in requested strength results in a drop in
physical rumble.

[ ... ]

> +static int gembird_joy_init(struct hid_device *hid)
> +{

[ ... ]

> +		joy = kzalloc(sizeof(struct gembird_joy_device), GFP_KERNEL);
> +		if (!joy)
> +			return -ENOMEM;

Does this memory leak when the device is disconnected or the driver is
unbound? The allocation doesn't use devm_kzalloc, and there isn't a
remove callback to free it. The input_ff_create_memless teardown
handles its own structures but not the user-provided private data.

> +
> +		dev = hidinput->input;
> +
> +		set_bit(FF_RUMBLE, dev->ffbit);
> +
> +		error = input_ff_create_memless(dev, joy, hid_gembird_joy_play);
> +		if (error) {
> +			kfree(joy);
> +			return error;
> +		}
> +
> +		joy->report = report;

Is there a window here where an ioctl could trigger a NULL pointer
dereference?

The device was already started earlier in gembird_joy_probe via
hid_hw_start, so userspace can issue an EVIOCSFF ioctl as soon as
input_ff_create_memless succeeds. If the ioctl triggers
hid_gembird_joy_play before joy->report is assigned on the next line,
the callback will try to access joy->report->field and dereference NULL.

[ ... ]

> +static int gembird_joy_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	int ret;
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "parse failed\n");
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
> +	if (ret) {
> +		hid_err(hdev, "hw start failed\n");
> +		return ret;
> +	}
> +
> +	gembird_joy_init(hdev);
> +
> +	return 0;
> +}

What happens if gembird_joy_init fails?

If input_ff_create_memless returns an error, gembird_joy_init
propagates it, but the return value is ignored here and probe succeeds.

Because set_bit(FF_RUMBLE, dev->ffbit) was already called, the device
remains registered and advertises rumble support. If userspace then issues
an EVIOCSFF ioctl, the kernel will allow it, which could lead to a panic
when it tries to dereference the missing force feedback structures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/b3bfa73b-0547-45f9-b6ae-f28815e6c632@gmx.at?part=1