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 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@redhat.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@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 + */ +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 +282,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: *