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

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?

-Emil