[RFC] drm: rework SET_MASTER and DROP_MASTER perm handling

Tue Feb 11 11:54:04 UTC 2020

On Mon, 10 Feb 2020 at 21:54, Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Mon, Feb 10, 2020 at 8:01 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >
> > 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.
>
> Hm ... I guess this works indeed. Or should. I'm mildly freaked out
> that we're checking for opener_pid == current->pid. Not sure how many
> other security assumptions we're breaking.
>
Today there are many ways to DoS your compositor - regardless if the
app uses logind or not.
As said in the other thread - we do refcount the pid. So its lifetime
should be preserved and the check will be fine.

> > 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.
>
> Given the above bug I think a solid igt for both the logind and the
> non-logind scenario is needed. We have some helpers to drop root and
> fork stuff and all that, so shouldn't be many lines.

The non-logind case would be pretty trivial. The logind one will take
a bit of time - I need to catch up on all the fancy helpers.

Thanks
Emil