Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --device=input permission #5481

Merged
merged 1 commit into from Sep 8, 2023
Merged

Add --device=input permission #5481

merged 1 commit into from Sep 8, 2023

Conversation

foresto
Copy link
Contributor

@foresto foresto commented Jul 16, 2023

This new permission exposes the host's /dev/input directory, providing
minimal game controller support without resorting to --device=all.

(It originally handled /dev/uinput and /dev/hidraw* as well, for the sake of motion controls, Steam Input, and similar advanced features, but that functionality has since been removed.)

I made this for my own use, but thought it might be a helpful contribution, since I've noticed multiple suggestions for something like it.

Ref:
#7
#4405 (comment)

@orowith2os
Copy link

This Wayland protocol is probably preferable: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/110

@smcv
Copy link
Collaborator

smcv commented Jul 16, 2023

any /dev/hidraw* nodes for which the user has read permission

This is not going to work for hotplugged game controllers: it will act on the list of game controller device nodes that already exist during sandbox setup, but can't work for game controllers that are hotplugged while the container already exists.

@foresto
Copy link
Contributor Author

foresto commented Jul 16, 2023

This Wayland protocol is probably preferable

My goal here is to provide a practical, immediately available way to avoid device=all on both X11 and Wayland. I don't think a Wayland protocol can do that, but if that protocol becomes an implementation at some point in the future, perhaps the two approaches could complement each other.

@foresto
Copy link
Contributor Author

foresto commented Jul 16, 2023

This is not going to work for hotplugged game controllers

That's true of hidraw devices, of course. I was under the impression that hotplug doesn't work for those even with device=all, due to netlink events not getting through. Is that not the case? (Or was it the case in the past but no longer?)

If that bit has been solved, then huzzah, but device=all remains a conspicuous hole in the sandbox that Flatpak famously promises.

Having device=input available would at least give users the option of closing that hole, with the understanding that they would have to connect their controllers before launching if they need raw device access. The current situation offers no choice: everyone is expected to expose their entire /dev tree, even if they don't hotplug their controllers or play any raw-input games. Judging by how often I see flatseal recommended to address problems like this, it seems clear that I'm not the only one uncomfortable with the allow-everything policy.

And for games that don't use raw devices, this would be a clear, easy, and far more appropriate manifest permission than device=all, requiring no user effort.

@smcv
Copy link
Collaborator

smcv commented Aug 19, 2023

I was under the impression that hotplug doesn't work for [hidraw devices] even with device=all, due to netlink events not getting through. Is that not the case?

It's partially true. If the sandboxed program uses libudev for device enumeration, then you're right, because the protocol between udevd and libudev for announcing new devices relies on those netlink events. (Strictly speaking, they do get through, but the indication that they were sent by uid 0 and therefore can be trusted is lost, so the client library doesn't and can't trust them.)

However, if the sandboxed program does device enumeration directly, not via libudev (by using inotify on /dev for change notification, and reading device information from /sys) then there is no dependency on udevd netlink events. Various projects (SDL, Wine, libmanette, the Godot engine and perhaps others) use that code path automatically if they are inside a Flatpak-style sandbox.

evdev devices (/dev/input) are the same as hidraw devices in this respect: hotplug can work for them, but only if the sandboxed program does not rely on libudev. In SDL, I contributed a non-libudev code path for evdev devices, and later one of my colleagues contributed the same thing for hidraw devices, so there were a few releases where evdev worked better than hidraw.

However, for a sandbox that needs to set up /dev ahead of time (like Flatpak must), because /dev/hidraw* are not in a subdirectory, the only way for newly-hotplugged hidraw devices to show up inside the sandbox is if all of /dev is shared.

I think it would be better for expectations management (and also a lot simpler!) if this branch covered /dev/input/ and maybe /dev/uinput, but not /dev/hidraw*. That way, with a sufficiently capable input library like SDL, all devices that initially work in the sandbox will also work when hotplugged. That wouldn't be enough for Steam, but as you say, it would be a step away from --device=all for less demanding input device users.

common/flatpak-run.c Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Aug 30, 2023

I don't think this should be exporting hidraw devices for the reasons discussed previously. Conveniently, that would also drop the most complicated bits of it.

If this only exported /dev/input/ and /dev/uinput then I would be inclined to merge it.

@smcv
Copy link
Collaborator

smcv commented Aug 30, 2023

Merge branch 'main' into device-input

Please rebase proposed branches onto main, instead of merging main into them.

@foresto
Copy link
Contributor Author

foresto commented Sep 6, 2023

Please rebase proposed branches onto main, instead of merging main into them.

Will do. What you saw was not my work, but github's, after I clicked the button that it offered to automatically update the branch. (Perhaps that button should be removed, or its default behavior changed?)

@foresto
Copy link
Contributor Author

foresto commented Sep 6, 2023

I think it would be better for expectations management (and also a lot simpler!) [...]

I hope the ~20 lines that do this work are easily understood, and pull their weight by making things simpler for users.

[...] if this branch covered /dev/input/ and maybe /dev/uinput, but not /dev/hidraw*. That way, with a sufficiently capable input library like SDL, all devices that initially work in the sandbox will also work when hotplugged.

If this only exported /dev/input/ and /dev/uinput then I would be inclined to merge it.

I'm tempted to take you up on that, but skipping hidraw devices entirely would be a disappointing sacrifice.

Why should people who don't need hotplug be denied the full safety and convenience that the input permission could offer? Should they really have to learn sysfs and udev, and develop their own launch scripts, to get this functionality? Flatpak is in a position to make it easy. I can't help thinking we could find another way to manage expectations.

Counterproposal:

We could print a message when device=input is used, noting that hotplugged devices require device=all for raw input. (It could be either printed by default or only when run in verbose mode.) That would be immediately visible if someone was troubleshooting a flatpak that didn't work as expected, and would serve as a guide/reminder to packagers during their initial testing.

(I have added that to my latest version.)

We could also recommend in the docs that manifest authors use device=all instead of device=input when packaging raw input apps. That would make things Just Work by default, and allow users wanting a more restricted environment to get it with an easy override. Other apps could use device=input for the best of both worlds, with no extra steps for anyone.

What do you think?

@orowith2os
Copy link

Why should people who don't need hotplug be denied the full safety and convenience that the input permission could offer? Should they really have to learn sysfs and udev, and develop their own launch scripts, to get this functionality?

Or they just add device=all. I can see having this functionality down the road, with hidraw, causing more problems than just "hidraw isn't passed through by default".

We could print a message when device=input is used, noting that hotplugged devices require device=all for raw input.

This is what the permissions themselves should specify, but like I said, could still end up causing more problems for managing the complaints that come of it.

We could also recommend in the docs that manifest authors use device=all instead of device=input when packaging raw input apps. That would make things Just Work by default, and allow users wanting a more restricted environment to get it with an easy override. Other apps could use device=input for the best of both worlds, with no extra steps for anyone.

That would work, and again, should be where those permissions are specified in the source.

@smcv
Copy link
Collaborator

smcv commented Sep 6, 2023

Perhaps that button should be removed, or its default behavior changed?

I'm sorry, I don't have control over Github's user interface. Some of its behaviours can only be configured by a repository owner (I am not one of those), and some are non-configurable and can only be changed by Microsoft.

Why should people who don't need hotplug be denied the full safety and convenience that the input permission could offer?

Other than a few integrated devices like the Steam Deck, game controllers are typically hotpluggable: this is just a fact about the hardware. I know from my involvement in supporting Steam on Linux that users of game controllers typically consider non-working hotplug to be a showstopper issue, but in a mount-on-container-start-based approach, there is no way that we can ever implement hotplug (as long as the hidraw device nodes are directly in /dev and not in a subdirectory). So I stand by my opinion that sticking to evdev (with hotplug possible, and all devices showing up in a timely way) is a better and easier to explain/understand short-term answer than partially offering raw HID, but only for the subset of devices that happen to have been connected when our container was launched, and losing the ability to do raw HID to those devices if the USB cable is unplugged or the Bluetooth connection drops.

It is already the case that access to raw HID device nodes is not given out to non-root by default, and it requires special configuration at system level (steam-devices or game-devices-udev or some similar thing). Similarly, /dev/uinput is root-only by default.

Thinking about it again, it would perhaps be better to omit /dev/uinput from this change too, and only have /dev/input/. That way, it'll be possible to give particularly demanding input users like Steam Input access to /dev/uinput and raw HID (via --device=all), while giving standalone games access to /dev/input/ but not giving them the ability to fake keyboard input (!) via /dev/uinput.

The long-term answer to dealing with input devices - especially raw HID, but probably /dev/input/ in the longer term too - is probably going to have to be some sort of service on the host system that proxies this stuff into the container (Wayland inputfd, or an input device portal, or something of that sort). If/when that happens, the bad news is that it will likely only work for games and game engines that can be updated to support it; but the good news is that if the design is at all reasonable, then it will have change-notification (therefore ability to do hotplug) from the beginning, and it should be relatively straightforward to update middleware libraries like SDL and Wine/Proton to support it (therefore making that functionality available to all games that use them in one go).

@smcv
Copy link
Collaborator

smcv commented Sep 6, 2023

We could print a message when device=input is used, noting that hotplugged devices require device=all for raw input. (It could be either printed by default or only when run in verbose mode.)

I don't think this is a good compromise. If the application only uses evdev (/dev/input/) then this is just noise, and if the application uses raw HID then it will still be left with half-working raw HID - good enough that packagers doing a superficial test will say "works for me, ship it", but also bad enough to generate extra support burden from unfixable issue reports.

When Flatpak adds a feature/permission flag, I think the design principle should be that the feature being enabled is something that is reasonably straightforward to explain and reason about, even if it's not necessarily everything that a user or application could possibly want. "Access to /dev/input/ devices" is straightforward to explain, and it's straightforward for an app maintainer with knowledge of how their app works to assess whether it's enough. If it's all that the app needs, then that access is enough, and if not, then the maintainer will have to consider requesting --device=all.

If the app can work with degraded functionality with only --device=dri and /dev/input/, but really wants --device=all for full functionality, then it would be valid to ask for --device=all --device=dri --device=input, giving a user or sysadmin the option to deny --device=all but continue to allow the other two.

We could also recommend in the docs that manifest authors use device=all instead of device=input when packaging raw input apps

What raw input apps do you have in mind, specifically? Is this basically Steam?

Steam Input and Steam Remote Play are very much a special case: as well as raw HID, they also require /dev/uinput access, which is normally root-only. From a security and integrity point of view, /dev/uinput is alarming, because it lets the caller create virtual gamepads, but also virtual keyboards and mice, which can send system-level input (for example there's nothing to stop Steam Input, or any other process with /dev/uinput open, from sending Ctrl+Alt+F6 and trying to log in at a virtual console).

I suspect that the long-term answer to this should be something involving libei/libeis and sending virtual keyboard/mouse input events at a higher level, for example through the remote-desktop portal; but in the shorter term, Steam Input needing --device=all seems more like being realistic about it having special privileges that most sandboxed apps don't need and don't get.

There is a limit to how strongly-sandboxed something can be when it has as many special requirements as Steam - Flatpak tries its best, and from a different direction so does Snap, but neither can really put something as complicated and "powerful" as Steam in a particularly robust sandbox without breaking some of what it does. If you're running Steam on a machine that is not solely used for gaming, the approach that I would suggest (and that I personally use) is having a separate user account (separate Unix uid) for playing games, switching to it with fast user switching or logout/login, and running Steam (either sandboxed or not) as that user account instead of as my "real" user account.

@orowith2os
Copy link

I suspect that the long-term answer to this should be something involving libei/libeis and sending virtual keyboard/mouse input events at a higher level, for example through the remote-desktop portal

I'd love it if you could contribute to https://gitlab.freedesktop.org/libinput/libei/-/issues/40 :)

@swick
Copy link
Contributor

swick commented Sep 6, 2023

FWIW, I've also been looking a bit into how to provide backwards compat for evdev joysticks which doesn't involve just exposing /dev/input. In particular what we want is that the evdev fd's will be muted as long as no surface of the app has focus.

One of the more interesting ideas is to use fuse to create a fs which emulates evdev in user space with all the ioctls and then bind mount that into flatpak. There are a few things that make it a bit ugly: the files are always regular files (will this be a problem if they behave exactly like evdev?), it adds latency, complexity and more load. So, maybe it's worth investigating more and actually try to just proxy a single evdev device via fuse and see if that works.

This new permission exposes the host's /dev/input directory, providing
minimal game controller support without resorting to --device=all.
@foresto
Copy link
Contributor Author

foresto commented Sep 7, 2023

tl;dr: I give up. Details below.

I know from my involvement in supporting Steam on Linux that users of game controllers typically consider non-working hotplug to be a showstopper issue,

The hidraw support in this change is not aimed at those users, and does not affect those users, since it doesn't try to be the default. It is aimed at people who want a more restrictive sandbox and don't mind plugging in their controllers beforehand to get it.

Thinking about it again, it would perhaps be better to omit /dev/uinput from this change too, and only have /dev/input/. That way, it'll be possible to give particularly demanding input users like Steam Input access to /dev/uinput and raw HID (via --device=all),

That would fail to solve problem that this patch was designed to address: Allowing people who use these input features to have them without resorting to device=all.

while giving standalone games access to /dev/input/ but not giving them the ability to fake keyboard input (!) via /dev/uinput.

This patch doesn't grant the ability to fake keyboard input; doing that requires special configuration at system level, as you said yourself. It merely gives a user who has already made that choice to have their decision respected by flatpak, without forcing them to also expose their entire /dev tree.

The long-term answer to dealing with input devices [...] is probably going to have to be some sort of service on the host system that proxies this stuff into the container

On this we agree. :) The work I have submitted here is useful today, without getting in the way of a more comprehensive long-term solution.

We could print a message when device=input is used, noting that hotplugged devices require device=all for raw input. [...] We could also recommend in the docs that manifest authors use device=all instead of device=input when packaging raw input apps.

If the application only uses evdev (/dev/input/) then this is just noise,

It's eight additional words on a message that already exists.

and if the application uses raw HID then it will still be left with half-working raw HID - good enough that packagers doing a superficial test will say "works for me, ship it", but also bad enough to generate extra support burden [...]

I think this is an unrealistic worry. It would only manifest if all of these things occurred:

  • The flatpak maintainer discovered device=input but ignored its documentation.
  • The flatpak maintainer neglected to test a use case (hotplug) that is common for games, as you said yourself.
  • The flatpak maintainer failed to notice the warning message.
  • The users deliberately installed udev rules (by hand or via steam-devices) to get raw or virtual device input.
  • The users failed to notice the warning message directing them to device=all.
  • The flatpak maintainer chose to ignore early user complaints instead of applying the easy-to-discover fix.

Moreover, extra support burden would be vanishingly unlikely for Steam (and other popular apps), since the flatpaks for those already exist and use device=all. Someone would have to go out of their way to change it, and all of the above mistakes would also have to happen.

[...] from unfixable issue reports.

What's unfixable? Simply reverting to device=all would fix it.

When Flatpak adds a feature/permission flag, I think the design principle should be that the feature being enabled is something that is reasonably straightforward to explain and reason about, even if it's not necessarily everything that a user or application could possibly want. "Access to /dev/input/ devices" is straightforward to explain,

We agree here, and I think "Note: Hotplugging raw input devices requires --device=all" is reasonably straightforward.

I also feel that sandboxing is, in principle, an imporant feature of flatpak (it's a key advantage over alternatives like appimage) but device=all is overused so broadly that the sandbox ends up laughable in practice. This problem can be reduced with well-chosen flatpak override options like the one I implemented here, allowing security-conscious people to follow the principle of least privilege. I believe this is the responsible thing to do, and that people who choose flatpak for its sandbox would appreciate it.

I would suggest (and that I personally use) is having a separate user account (separate Unix uid) for playing games,

Well, yes, that's good advice in general (I do it, too) but it doesn't solve the problem this patch was designed to address. I would suggest doing both.

@foresto
Copy link
Contributor Author

foresto commented Sep 7, 2023

I remain convinced that my full patch, including hidraw and uinput, is valuable and not likely to create a support burden. (It is an opt-in feature, after all.)

But I'm tired of arguing, so I have gutted it as smcv requested. Perhaps the removed parts can be reconsidered in the future.

@smcv smcv merged commit 738a0b1 into flatpak:main Sep 8, 2023
9 checks passed
@smcv
Copy link
Collaborator

smcv commented Sep 8, 2023

Thanks, merged.

Perhaps the removed parts can be reconsidered in the future.

Perhaps. Adding a limited version first, with the option to expand it later, is the better direction to go in: this way, we have the option to expand the scope of the permission without regressing anything that already worked, whereas if we had added a more powerful version first, we wouldn't be able to go back to a more limited version except by adding it under a different name (--device=evdev or something).

hfiguiere added a commit to hfiguiere/flatpak that referenced this pull request Feb 11, 2024
Follow-up to flatpak#5481

Signed-off-by: Hubert Figuière <[email protected]>
hfiguiere added a commit to hfiguiere/flatpak that referenced this pull request Feb 11, 2024
Follow-up to flatpak#5481

Signed-off-by: Hubert Figuière <[email protected]>
TingPing pushed a commit that referenced this pull request Feb 11, 2024
Follow-up to #5481

Signed-off-by: Hubert Figuière <[email protected]>
GeorgesStavracas pushed a commit to GeorgesStavracas/flatpak that referenced this pull request Apr 26, 2024
Follow-up to flatpak#5481

Signed-off-by: Hubert Figuière <[email protected]>
tsunamistate added a commit to flathub/org.diasurgical.DevilutionX that referenced this pull request May 4, 2024
Turns out this has been available since 2023
flatpak/flatpak#5481
This gets rid of overly permissive `device=all`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants