implement memory for 16 rumble effects

[kh0nsu]

Proton/wine and possibly other applications seem to create a dummy rumble effect when a game starts, and another effect later when actually trying to trigger the rumble. A memory of 1 means only the dummy effect can be created, so you don't get any rumble. Chromium browsers can also grab the effect slot if open, preventing anything else from using it.

This patch adds an array for an arbitrary 10 effects, which should cover every sane application. I've only tried one game with proton, fftest (though the 'weak' rumble was stronger than the 'strong' one...?) and a gamepad viewer with Chromium, but it seems to work as expected.

This should resolve #187 and possibly others.

Feel free to delete the comments and debug messages.

[kh0nsu]

I agree it's technically unsafe, hence the comment, though the ID assignment looks like it hasn't changed in 15 years and I don't see why they would change it now... The "long term solution" might be to use an std::map and just pass in the highest allowed maximum...

Anyway, a limit of 16 and some bounds checking sounds good. I don't really get how github works, can anyone propose a commit to add to the PR? 🤔 Otherwise if someone fixes it before me and puts up another PR I can delete this one.

[kakra]

can anyone propose a commit to add to the PR?

Since you created the PR, just push to the same branch... The PR will be updated. If you want to squash or amend your commits, do that locally, and the use git push kh0nsu --force-with-lease.

[kakra]

I agree it's technically unsafe, hence the comment, though the ID assignment looks like it hasn't changed in 15 years

This is nothing to discuss about: The FF interface can be accessed from user sessions, and xow runs as root, so a process could easily cause a buffer overflow and inject code. It has nothing to do with what values have been passed in the last 15 years. If you program in C/C++, you're forced to do your proper checks, and not rely on some convenience.

[kh0nsu]

xow runs as root

Uh, no it doesn't? The udev rules allow any user to run it, and the systemd script runs it as a dynamic user.

The FF interface can be accessed from user sessions

If you're imagining that another program can just pass in an arbitrary ID to xow, that's not how it works. The upload must be performed with -1 (ie. kernel-assigned ID), or an ID already assigned to the user, which will be within range.

I'm very interested to see a proof-of-concept for how a non-root user can break something like this.

[kakra]

Uh, no it doesn't? The udev rules allow any user to run it, and the systemd script runs it as a dynamic user.

I think we can comply that this doesn't justify not to do sanity checks... Running it non-root is just one of multiple security measures. Last time I checked, there have been problems reported when running it in a user session.

If you're imagining that another program can just pass in an arbitrary ID to xow, that's not how it works. The upload must be performed with -1 (ie. kernel-assigned ID), or an ID already assigned to the user, which will be within range.

I'm not sure what the kernel limits are here but let's think about two situations: We have no control over changes that may go into the kernel, the limits may change. And nothing prevents an attacker from just spamming the effect upload. Even if an arbitrary ID cannot just directly passed, there may be indirect ways - and we also have no control over bypassing or adhering security measures from those upper layers. So it's still better to check for sanity. If the we had control over the callers, the situation would be different.

I'm very interested to see a proof-of-concept for how a non-root user can break something like this.

I'm no expert in constructing attack situations but I'm looking into hardening code a lot especially in web development, and relying on external factors about what could be passed to our code or couldn't is probably one of the worst things to do. Such assumptions usually break down with the security and safety of the callers.

We could bypass this situation if we could rely on ff-memless to do the work for us. As a uinput driver, however, we cannot use that interface. But xow could instead use uhid and inject a HID driver into the kernel from user-space. Then we would get rumble calls directly from ff-memless on the HID channel and have all the benefits that ff-memless implements without much to worry. Actually, that's what I plan to add to hid-xpadneo: It will get a user-space helper to inject GIP devices as HID into the kernel very similar to how bluez does it.

BTW:

• https://github.com/torvalds/linux/blob/fe91c4725aeed35023ba4f7a1e1adfebb6878c23/drivers/input/ff-memless.c#L29 The kernel ff-memless driver has 16 effect slots
• https://github.com/torvalds/linux/blob/fe91c4725aeed35023ba4f7a1e1adfebb6878c23/drivers/input/ff-memless.c#L530 It creates a force-feedback input device with 16 slots
• https://github.com/torvalds/linux/blob/fe91c4725aeed35023ba4f7a1e1adfebb6878c23/drivers/input/ff-core.c#L131 input_ff_upload returns -ENOSPC when -1 is used and no more slots are free
• https://github.com/torvalds/linux/blob/fe91c4725aeed35023ba4f7a1e1adfebb6878c23/drivers/input/ff-core.c#L141 input_ff_upload checks access, check_effect_access does bounds (and owner) checking
• https://github.com/medusalix/xow/blob/a396e3fd3f1334e1b9edd011931316ccf15ce431/controller/input.cpp#L108 xow passes a setup parameter to the kernel, so we should be protected by ff-core in theory (which is actually your argument)

But still, nothing guards who actually calls us. And we don't control the callers. The kernel guards all its function calls, and ff-memless doesn't really care as it only looks if a slot is active or isn't, it always looks at all 16 slots because it will always combines all active effects into one single effect for the memless device, emulating what can be emulated on a simple rumble device. Our design is currently different where you could select the effect to be played (where only rumble is supported). And if we went that far that we would be replicating what ff-memless does, I think it's more efficient to just make xow into a uhid driver instead, and let ff-memless do the magic directly and go back to supporting just one effect.

However, I'm less concerned with checking the owner or for erased effects: It should do no harm if we omit that except someone could replace your effects, or play erased effects. One could probably construct an example where a replaced rumble effect could become a threat - but that's probably super theoretical, especially given the target audience.