Re: [PATCH] drm: rework SET_MASTER and DROP_MASTER perm handling

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Tue, Mar 17, 2020 at 1:26 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Mon, 2 Mar 2020 at 18:29, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > On Wed, 19 Feb 2020 at 13:27, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >
> > > From: Emil Velikov <emil.velikov@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 ensures that we:
> > >  - do not regress the systemd-logind style of DRM_MASTER arbitrator
> > >  - allow applications which do not use systemd-logind to drop their
> > > master capabilities (and regain them at later point) ... w/o running as
> > > root.
> > >
> > > See the comment above drm_master_check_perm() for more details.
> > >
> > > v1:
> > >  - Tweak wording, fixup all checks, add igt test
> > >
> > > Cc: Adam Jackson <ajax@redhat.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > > Testcase: igt/core_setmaster/master-drop-set-user
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > ---
> > >  drivers/gpu/drm/drm_auth.c  | 62 +++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_ioctl.c |  4 +--
> > >  include/drm/drm_file.h      | 11 +++++++
> > >  3 files changed, 75 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> > > index cc9acd986c68..b26986bca271 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,67 @@ 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. 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.
> > > + *
> > > + *
> > > + * 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.
> > > + *
> > > + * 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
> > > + */
> > > +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;
> > > +
> > > +       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);
> > > +       if (ret)
> > > +               goto out_unlock;
> > > +
> > > +       ret = -EINVAL;
> > >         if (!drm_is_current_master(file_priv))
> > >                 goto out_unlock;
> > >
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index 9e41972c4bbc..73e31dd4e442 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -599,8 +599,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> > >         DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_SET_SAREA_CTX, drm_legacy_setsareactx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
> > >         DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH),
> > >
> > > -       DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, DRM_ROOT_ONLY),
> > > -       DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_ROOT_ONLY),
> > > +       DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, 0),
> > > +       DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, 0),
> > >
> > >         DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY),
> > >         DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
> > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > > index 19df8028a6c4..c4746c9d3619 100644
> > > --- a/include/drm/drm_file.h
> > > +++ b/include/drm/drm_file.h
> > > @@ -201,6 +201,17 @@ struct drm_file {
> > >          */
> > >         bool writeback_connectors;
> > >
> > > +       /**
> > > +        * @was_master:
> > > +        *
> > > +        * This client has or had, master capability. Protected by struct
> > > +        * &drm_device.master_mutex.
> > > +        *
> > > +        * This is used to ensure that CAP_SYS_ADMIN is not enforced, if the
> > > +        * client is or was master in the past.
> > > +        */
> > > +       bool was_master;
> > > +
> > >         /**
> > >          * @is_master:
> > >          *
> > > --
> > > 2.25.0
> > >
> >
> > Humble poke?
> >
> Another humble poke?
>
> Daniel you seemed on the fence for the RFC.
> With the questions raised by Pekka and addressed by yours truly, can
> you please review this patch?

Just wanted to make sure the igt are address and things look
reasonable for uapi. Jesse just pinged me on irc that CrOS wants
this/needs this, I think best if one of the cros people (we should
have a bunch here) reviews this and then you can push it.

> The IGT tests have been in the i915-CI for a while now.

Yeah thanks a lot for doing that.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel