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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Fri, May 08, 2026 at 10:51:30PM +0000, sashiko-bot@kernel.org wrote:
> 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.

Sashiko is wrong here.

> 
> > +
> > +		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.

Sashiko is wrong here as well. The device is not accessible from
userspace until it is registered.

> 
> [ ... ]
> 
> > +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.
> 

Need to have error handling.

Thanks.

-- 
Dmitry