implement memory for 16 rumble effects #191
Conversation
controller/input.cpp
Outdated
| @@ -163,7 +169,7 @@ void InputDevice::handleFeedbackUpload(uint32_t id) | |||
| return; | |||
| } | |||
|
|
|||
| effect = upload.effect; | |||
| effects[upload.effect.id] = upload.effect; //should really check the id first but the driver always seems to assign the first available from 0. | |||
There was a problem hiding this comment.
This probably should not be done without bounds checking... Especially since the array has a static size. You could look at kernel's ff-memless to look how many effect slots it supports. I believe it was 16. That way, both would behave the same.
But I also think that ff-memless combines effects with each other. With this implementation, only one effect at a time could play and would override the others.
controller/input.cpp
Outdated
| @@ -191,7 +201,7 @@ void InputDevice::handleFeedbackErase(uint32_t id) | |||
| return; | |||
| } | |||
|
|
|||
| effect = {}; | |||
| effects[erase.effect_id] = {}; | |||
There was a problem hiding this comment.
Same here, bounds checking is missing. Maybe create accessor function instead of accessing the array directly, that is: make it private, create functions setEffect and getEffect.
There was a problem hiding this comment.
Using accessor function will make it easier later to change the implementation to maps if someone wants to. But I think a static array is perfectly okay, ff-memless does probably something similar: It also has a limited amount of memory. We can just skip the overhead from map lookups, a static array should be okay. Still using accessor function allows for limiting changes to code to one central place.
controller/input.h
Outdated
| @@ -27,6 +27,8 @@ | |||
| #include <stdexcept> | |||
| #include <linux/uinput.h> | |||
|
|
|||
| #define INPUT_MAX_FF_EFFECTS 10 | |||
There was a problem hiding this comment.
As already written, this should probably match with what ff-memless in the kernel supports.
|
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. |
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 |
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. |
| @@ -91,7 +93,7 @@ class InputDevice | |||
| InterruptibleReader eventReader; | |||
| std::thread eventThread; | |||
|
|
|||
| ff_effect effect = {}; | |||
| ff_effect effects[INPUT_MAX_FF_EFFECTS] = {}; | |||
| uint16_t effectGain = 0xffff; | |||
| FeedbackReceived feedbackReceived; | |||
There was a problem hiding this comment.
Look, this is the problematic code: If you write out-of-bounds, you can overwrite the function pointer in feedbackReceived - thus provoking a jump to unknown code which the attacker could also conveniently pass through out-of-bounds writes.
Uh, no it doesn't? The udev rules allow any user to run it, and the systemd script runs it as a dynamic user.
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. |
kh0nsu
changed the title
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.
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 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:
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. |
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.