[PATCH v7] unstable/drm-lease: DRM lease protocol support

[Daniel Vetter]

On Mon, Oct 21, 2019 at 10:48 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Fri, 18 Oct 2019 17:47:49 +0200
> Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>
> > On Fri, Oct 18, 2019 at 4:34 PM Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > >
> > > On Fri, 18 Oct 2019 16:19:33 +0200
> > > Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > >
> > > > On Fri, Oct 18, 2019 at 3:43 PM Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > > > >
> > > > > On Fri, 18 Oct 2019 07:54:50 -0400
> > > > > "Drew DeVault" <sir at cmpwn.com> wrote:
> > > > >
> > > > > > On Fri Oct 18, 2019 at 12:21 PM Pekka Paalanen wrote:
> > > > > > > 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.
> > > > > > >
> > > > > > > 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?
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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.)
> > > >
> > > > So the rules are (if I'm not making a mistake)
> > > > - If you're not CAP_SYS_ADMIN you can't get/drop_master.
> > >
> > > 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.
> > >
> > > > - 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.
> > >
> > > That would close the loophole that Ville mentioned, too, otherwise
> > > distributions should aim to not give permissions to open the DRM device
> > > node.
> >
> > 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 would guess https://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
> It's fairly low traffic nowadays.

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

> > > > - I thought you can always re-open your own fd through proc? Which
> > > > should be good enough for this use-case here.
> > >
> > > We can? And that creates a new file description the same way as open()
> > > in the original device node?
> >
> > I dreamed, it's just a normal symlink, nothing fancy.
>
> 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?

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch