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

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?

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

Thanks Emil