[PATCH] drm: rework SET_MASTER and DROP_MASTER perm handling

[Emil Velikov]

On Fri, 6 Mar 2020 at 14:00, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Wed, 19 Feb 2020 13:27:28 +0000
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> > From: Emil Velikov <emil.velikov at collabora.com>
> >
>
> ...
>
> > +/*
> > + * In the olden days the SET/DROP_MASTER ioctls used to return EACCES when
> > + * CAP_SYS_ADMIN was not set. This was used to prevent rogue applications
> > + * from becoming master and/or failing to release it.
> > + *
> > + * At the same time, the first client (for a given VT) is _always_ master.
> > + * Thus in order for the ioctls to succeed, one had to _explicitly_ run the
> > + * application as root or flip the setuid bit.
> > + *
> > + * If the CAP_SYS_ADMIN was missing, no other client could become master...
> > + * EVER :-( Leading to a) the graphics session dying badly or b) a completely
> > + * locked session.
> > + *
>
> Hi,
>
> sorry I had to trim this email harshly, but Google did not want to
> deliver it otherwise.
>
> I agree that being able to drop master without CAP_SYS_ADMIN sounds
> like a good thing.
>
> > + *
> > + * As some point systemd-logind was introduced to orchestrate and delegate
> > + * master as applicable. It does so by opening the fd and passing it to users
> > + * while in itself logind a) does the set/drop master per users' request and
> > + * b)  * implicitly drops master on VT switch.
> > + *
> > + * Even though logind looks like the future, there are a few issues:
> > + *  - using it is not possible on some platforms
> > + *  - applications may not be updated to use it,
> > + *  - any client which fails to drop master* can DoS the application using
> > + * logind, to a varying degree.
> > + *
> > + * * Either due missing CAP_SYS_ADMIN or simply not calling DROP_MASTER.
> > + *
> > + *
> > + * Here we implement the next best thing:
> > + *  - ensure the logind style of fd passing works unchanged, and
> > + *  - allow a client to drop/set master, iff it is/was master at a given point
> > + * in time.
>
> I understand the drop master part, because it is needed to get rid of
> apps that accidentally gain DRM master because they were the first one
> to open the DRM device (on a particular VT?). It could happen e.g. if a
> Wayland client is inspecting DRM devices to figure what it wants to
> lease while the user has VT-switched to a text-mode VT, I guess. E.g.
> starting a Wayland VR compositor from a VT for whatever reason.
>
> The set master without CAP_SYS_ADMIN part I don't understand.
>
As you point out application can drop master for various reasons. One
of which may be to say spawn another program which requires master for
_non_ modeset reasons. For example:
 - amdgpu: create a renderer and use the context/process priority override IOCTL
 - vmwgfx: stream claim/unref (amongst others).

Another case to consider is classic X or Wayland compositor. With
CAP_SYS_ADMIN for DROP_MASTER removed, yet retained in SET_MASTER, the
IOCTL will fail. Thus:
 - weston results in frozen session and session switching (have to
force kill weston or sudo loginctl kill-session)
 - depending on the driver, X will work or crash

To make this clearer I'll include //comment sections in the code.

// comment
To ensure the application can reclaim its master status, the tweaked
CAP_SYS_ADMIN handling is needed for both IOCTLs. Otherwise X or
Wayland compositors may freeze or crash as SET_MASTER fails.
// comment


> > + *
> > + * As a result this fixes, the following when using root-less build w/o logind
>
> Why is non-root without any logind-equivalent a use case that should
> work?
>
// comment
Some platforms don't have equivalent (Android, CrOS, some BSDs), yet
root is required _solely_ for DROP/SET MASTER. So tweaking the
requirement sounds perfectly reasonable.
// comment

> Why did DRM set/drop master use to require CAP_SYS_ADMIN in the first
> place?
>
I imagine something else could have been introduced instead. Although
I cannot find any details or discussion on the topic.

> What else happens if we allow DRM set master more than just for
> CAP_SYS_ADMIN?
>
If we're talking about removing CAP_SYS_ADMIN all together:
 - screen scraping by any random application
 - dead trivial way to DoS your compositor


> Does this interact with DRM leasing?
>
> Looks like drmIsMaster() should be unaffected at least:
> https://patchwork.kernel.org/patch/10776525/
>
Correct, both are unaffected. All the leasing IOCTLs happen by the
active true (aka non-lease) master.


> > + * - startx - some drivers work fine regardless
> > + * - weston
> > + * - various compositors based on wlroots
> > + */
> > +static int
> > +drm_master_check_perm(struct drm_device *dev, struct drm_file *file_priv)
> > +{
> > +     if (file_priv->pid == task_pid(current) && file_priv->was_master)
> > +             return 0;
>
> In case a helper e.g. logind opens the device, is file_priv->pid then
> referring to logind regardless of what happens afterwards?
>
Correct.

> > +
> > +     if (!capable(CAP_SYS_ADMIN))
> > +             return -EACCES;
> > +
> > +     return 0;
> > +}
> > +
> >  int drm_setmaster_ioctl(struct drm_device *dev, void *data,
> >                       struct drm_file *file_priv)
> >  {
> >       int ret = 0;
> >
> >       mutex_lock(&dev->master_mutex);
> > +
> > +     ret = drm_master_check_perm(dev, file_priv);
> > +     if (ret)
> > +             goto out_unlock;
> > +
> >       if (drm_is_current_master(file_priv))
> >               goto out_unlock;
> >
> > @@ -229,6 +285,12 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> >       int ret = -EINVAL;
> >
> >       mutex_lock(&dev->master_mutex);
> > +
> > +     ret = drm_master_check_perm(dev, file_priv);
>
> Why does drop-master need any kind of permission check? Why could it
> not be free for all?
>
Consider the arbitrator usecase - be that logind, Xorg (in ancient
times) or otherwise.

// comment
DROP_MASTER cannot be free for all, as any (say logind) user can:
 - can DoS/crash the arbitrator
 - open the node, become master implicitly and cause issues
// comment

I've added an IGT subtest to ensure this does not happen.

Let me know if I should include anything more to the commit, than the
above comment sections.

Thanks

-Emil