[RFC] drm: rework SET_MASTER and DROP_MASTER perm handling

[Emil Velikov]

Thanks for having a look Daniel.

On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov at collabora.com>
> >
> > This commit reworks the permission handling of the two ioctls. In
> > particular it enforced the CAP_SYS_ADMIN check only, if:
> >  - we're issuing the ioctl from process other than the one which opened
> > the node, and
> >  - we are, or were master in the past
> >
> > This allows for any application which cannot rely on systemd-logind
> > being present (for whichever reason), to drop it's master capabilities
> > (and regain them at later point) w/o being ran as root.
> >
> > See the comment above drm_master_check_perm() for more details.
> >
> > Cc: Adam Jackson <ajax at redhat.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > ---
> > This effectively supersedes an earlier patch [1] incorporating ajax's
> > feedback (from IRC ages ago).
> >
> > [1] https://patchwork.freedesktop.org/patch/268977/
> > ---
> >  drivers/gpu/drm/drm_auth.c  | 59 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_ioctl.c |  4 +--
> >  include/drm/drm_file.h      | 11 +++++++
> >  3 files changed, 72 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> > index cc9acd986c68..01d9e35c0106 100644
> > --- a/drivers/gpu/drm/drm_auth.c
> > +++ b/drivers/gpu/drm/drm_auth.c
> > @@ -135,6 +135,7 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv,
> >               }
> >       }
> >
> > +     fpriv->was_master = (ret == 0);
> >       return ret;
> >  }
> >
> > @@ -179,12 +180,64 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> >       return ret;
> >  }
> >
> > +/*
> > + * In the olden days the SET/DROP_MASTER ioctls used to return EACCES when
> > + * CAP_SYS_ADMIN was not set.
> > + *
> > + * Even though the first client is _always_ master, it also had to be run as
> > + * root, otherwise SET/DROP_MASTER would fail. In those cases no other client
> > + * could become master ... EVER.
> > + *
> > + * Resulting in a) the graphics session dying badly or b) a completely locked
> > + * session :-\
> > + *
> > + * 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) 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 obstacles:
> > + *  - using it is not possible on some platforms, or
> > + *  - 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 root permission or simply not calling DROP_MASTER.
> > + *
> > + *
> > + * Here we implement the next best thing:
> > + *   We enforce the CAP_SYS_ADMIN check only if the client was not a master
> > + * before. We distinguish between the original master client (say logind) and
> > + * another client which has the fd passed (say Xorg) by comparing the pids.
> > + *
> > + * As a result this fixes, the following when using root-less build w/o logind
> > + * - startx - some drivers work fine regardless
> > + * - weston
> > + * - various compositors based on wlroots
> > + */
>
> I think this breaks logind security. With logind no compositor can open
> the device node directly, hence no compositor can accidentally become the
> master and block everyone else.
>
I've explicitly considered this case. AFAICT this patch does not
change any of the contract.
If you think there's a scenario where things have broken, please let me know.

> And for the vt switch logind is the only one that can grant master rights,
> and it can make sure that the right compositor gets them. And if the old
> compositor is non-cooperating, it can also forcefully remove master
> rights.
>
Yes logind does set/drop master on VT switch, session setup/teardown, etc.

To take this a step further, there is no logind API or dbus method for
compositors to only set/drop master.
Thus logind ensures that compositors are in sane state.

> But with this here we lift this restriction if a compositor has ever been
> master. So the following thing could happen:
> - We have 3 compositors for different users C1, C2, C3
> - We switch from C1 to C2
> - While we switch no one is master for a moment, which means C3 could
>   sneak in and do a quick setmaster, and become master
> - Everything would come crashing done since logind believes it already
>   revoked master for C1, but somehow it now cant grant master to C2
>
Does this scenario consider that all three compositors are logind users?
If so, C3 should not be able to set or drop master. Since it got its
fd from logind:

 - `file_priv->pid` will point to systemd-logind, and
 - `task_pid(current)` will point to the respective compositor

-> EACCES will be returned to any compositor calling drmSetMaster.

Regardless of my patch, C3 can open() and simply not release the master.
Assuming it's the first DRM client of course - say switch to VTx +
login + start C3.

I've been lucky enough to spot various ways to softlock my system...
even when the compositor is using logind ;-)
If you're really interested I can share, but I'm worried that people
may see them as bashing at logind.

> I'm not sure we can even support these two models at the same time.
>
> > +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)
>
> Isn't this a typo? Why should we only allow this if the opener is someone
> else ... that looks like the logind approach? Or is my bolean logic parser
> broken again.
>
Thanks for spotting it. Indeed that should be:

if (file_priv->pid == task_pid(current) && file_priv->was_master)
    return 0;


Modulo any objections, I'll do proper testing and submit a non RFC version.
The inline comments will explicitly mention your concerns and why the
patch is safe.


Thanks
Emil