On Thu, 17 Oct 2019 13:45:27 -0400 Drew DeVault sir@cmpwn.com wrote:
Hi Drew,
thanks for this, I really hope it works out since the protocol is so neat and tidy now.
One thing I did not know the last time was that apparently systemd-logind may not like to give out non-master DRM fds. That might need fixing in logind implementations. I hope someone would step up to look into that.
I'm CC'ing dri-devel and Daniel Vetter to get some kernel side review for the non-master DRM fd idea:
This protocol aims to deliver a harmless "read-only" DRM device file description to a client, so that the client can enumerate all DRM resources, fetch EDID and other properties to be able to decide which connector it would want to lease. The client should not be able to change any KMS state through this fd, and it should not be able to e.g. spy on display contents. The assumption is that a non-master DRM fd from a fresh open() would be fine for this, but is it?
If it is not, could we make one that is? Simply giving out an fd for the client to inspect with standard DRM ioctls is just so convenient.
**
I wouldn't mind if the below links were part of the proper commit message.
Marius' email address probably needs refreshing?
Ok, that takes care of the multi-card case. It also nicely identifies which device a connector is from.
Don't we need a protocol error defined for the case, where the client uses a zwp_drm_lease_connector_v1 from a different global? That is, attempting to lease a connector from the wrong device.
If that was an error, then it would make sure that one cannot lease a mix across multiple devices in one go. I don't think leasing a mix across multiple devices could work, given only one DRM lease fd should be delivered.
Isn't the wording about drmModeGetLease() wrong here? The client can only see the leased resources through that fd, no matter what it does, right?
This seems to be in a very good shape now.
Thanks, pq
On Fri Oct 18, 2019 at 12:21 PM Pekka Paalanen wrote:
What I do for wlroots is call drmGetDeviceNameFromFd2, which returns the path to the device file, and then I open() it and use drmIsMaster/drmDropMaster to make sure it's not a master fd. This seems to work correctly in practice.
On Fri, 18 Oct 2019 07:54:50 -0400 "Drew DeVault" sir@cmpwn.com wrote:
That is nice.
Personally I'm specifically worried about a setup where the user has no access permissions to open the DRM device node directly, as is (or should be) the case with input devices.
However, since DRM has the master concept which input devices do not, maybe there is no reason to prevent a normal user from opening a DRM device directly. That is, if our assumption that a non-master DRM fd is harmless holds.
(Wayland display servers usually run as a normal user, while logind or another service runs with elevated privileges and opens input and DRM devices on behalf of the display server.)
Thanks, pq
On Fri, Oct 18, 2019 at 04:43:29PM +0300, Pekka Paalanen wrote:
If there is no master then the first guy to open the device automagically becomes the master. Maybe a theoretical DoS vector if you can sneak in and open the device between dropmaster/setmaster?
On Fri, Oct 18, 2019 at 3:43 PM Pekka Paalanen ppaalanen@gmail.com wrote:
So the rules are (if I'm not making a mistake) - If you're not CAP_SYS_ADMIN you can't get/drop_master.
- This is a bit awkward, since non-root can become a master, when there's no other master right at this point. So if you want to be able to do this, we should probably clarify this part of the uapi somehow (either de-priv drop_master or make sure non-root can't become master, but the latter probably will break something somewhere). Plus igts to lock down this behaviour. Note that if logind does a vt switch there's a race window where no one is master and you might be able to squeeze in. So perhaps we do want to stop this behaviour and require CAP_SYS_ADMIN to become master, even accidentally.
- I thought you can always re-open your own fd through proc? Which should be good enough for this use-case here.
- Non-master primary node should indeed give you all the GET* ioctls for kms, and nothing else useful or at least dangerous (you might be able to render with that thing). Just make sure you dont authenticate that new fd. Again maybe we should clarify our docs a bit to make this use case official.
Cheers, Daniel
On Fri, 18 Oct 2019 16:19:33 +0200 Daniel Vetter daniel.vetter@ffwll.ch wrote:
Hi,
not able to drop, yikes. So if someone pokes the Wayland DRM leasing interface while the display server is VT switched away (does not have DRM master), and maybe no-one else has DRM master either (you're hacking in VT text mode), then a new DRM fd would be master with no way out?
So Wayland display servers should make sure they have master themselves before sending a supposedly non-master DRM fd to anyone else. I wonder if the Wayland protocol extension needs to consider that the compositor might not be able to send any fd soon. Being able to defer sending the fd should probably be mentioned in the protocol spec, so that clients do not expect a simple roundtrip to be enough to ensure the fd has arrived.
That would close the loophole that Ville mentioned, too, otherwise distributions should aim to not give permissions to open the DRM device node.
We can? And that creates a new file description the same way as open() in the original device node?
Does that avoid becoming master in the above VT-switched-away scenario?
Awesome, thanks, pq
Regarding hotplugging, the Wayland compositor is probably keeping track of hotplugs itself and withdrawing/offering connectors as appropriate. Also, when the lease is issued, the compositor withdraws that connector. For the client, upon hotplug I imagine the DRM asks start to fail, and it handles that accordingly (presumably it'll close the lease, if the compositor hasn't already, and wait for it to come back, or just exit).
When a hotplug of a leased connector happens on the compositor side, it can notice this and exercise its descretion in the implementation. I think the current text of the protocol supports this view.
On Fri Oct 18, 2019 at 5:34 PM Pekka Paalanen wrote:
fwiw I have an assert(!drmIsMaster(fd)); before I send it to the client, just to be safe. But this may be misguided, since apparently if it ends up with a master node drmDropMaster(fd) won't work. I kind of find this hard to believe, if there's only ever one master, and the master cannot drop master, then why does this ioctl even exist?
When you VT switch away, the Wayland compositor is no longer necessarily able to lease those displays anyway - it's not the master anymore. So it should withdraw them, and in case of a race it'll reject the lease request.
On Fri, 2019-10-18 at 10:43 -0400, Drew DeVault wrote:
Right. Whether it waits or quits should be the decision of the client, and I'd like there to be a good way to "wait for it to come back" (or to appear, initially). If the compositor sends a new zwp_drm_lease_manager_v1.connector event after the HMD connector becomes leasable (again), that should be good enough.
[...]
Right. On VT switch away, revoking all leases and disabling those connectors is the only sensible thing the compositor can do.
However, that is completely independent from the sending the drm_fd event. The spec currently says: "The compositor will send this event when the zwp_drm_lease_manager_v1 global is bound.". But if the client binds the global while the compositor doesn't have DRM master privilege, and if it is not possible to securely produce a non-master drm_fd to send at this time, maybe the sentence should be changed to "The compositor will send this event some time after the zwp_drm_lease_manager_v1 global is bound."?
regards Philipp
On Fri, 18 Oct 2019 10:43:16 -0400 "Drew DeVault" sir@cmpwn.com wrote:
DRM KMS operations do not fail merely because a connector becomes disconnected. You can even force on a connector that is initially disconnected.
If you mean on revoking the lease or losing DRM master, yes, then I'd expect KMS ioctls start to fail.
But is disconnection reason enough to revoke the lease? I guess we shall see.
Ok, so the expectation is that a compositor does not offer disconnected connectors, and withdraws offered but then disconnected connectors, and that it sends offers for connectors that become connected while leasable. I suppose that is reasonable, I still think a sentence suggesting towards such behaviour would be in place.
Btw. there is more to hotplug events than just connected/disconnected: link status changes, and HDCP status changes. I suspect more is coming, too. At some point we might need to add a hotplug event in the protocol, but I think that is easy to do even after stabilization.
Thanks, pq
On Fri, Oct 18, 2019 at 4:34 PM Pekka Paalanen ppaalanen@gmail.com wrote:
I'm kinda wondering whether we have to do this as a security fix, with maybe a module option to get the old behaviour back for those who need/want that. But I don't even know whom/where to ping for logind folks ...
I dreamed, it's just a normal symlink, nothing fancy.
Does that avoid becoming master in the above VT-switched-away scenario?
Would be a reopen like open(3), so same problem until we fix that. -Daniel
On Fri, 18 Oct 2019 17:47:49 +0200 Daniel Vetter daniel.vetter@ffwll.ch wrote:
I would guess https://lists.freedesktop.org/mailman/listinfo/systemd-devel
It's fairly low traffic nowadays.
D'oh.
So we have two options: ensure logind is happy to deliver also non-master DRM fds, or prohibit an open() on a DRM device node from becoming master without CAP_SYS_ADMIN or something, right?
Drew does have a point though: if a non-CAP_SYS_ADMIN process gains DRM master, it has no way to drop master, does it? Which means it's impossible to VT-switch away from it and have something else gain master?
Still, I can see how someone could rely on gaining master via open() and as non-root.
Thanks, pq
On Mon, Oct 21, 2019 at 10:48 AM Pekka Paalanen ppaalanen@gmail.com wrote:
Hm I thought the moved all over to github. Adding to cc, in case that still gets somewhere.
Super short summary: While vt-switching there's a race window where anyone who can open the primary drm fd can become drm master, even if they're non-root. And the only way to fix up that mess is with close() (since the dropmaster ioctl is root-only).
Yeah I think it breaks vt-switching pretty bad.
Still, I can see how someone could rely on gaining master via open() and as non-root.
Yeah it's the "started some program from the console/ssh with nothing else running" use case. -Daniel
So, I'm not sure what the action items are here. It seems like we might have uncovered a potentially icky race condition in Linux, but that this protocol is not really affected.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Drew DeVault -
Pekka Paalanen -
Philipp Zabel -
Ville Syrjälä