On Wed, 19 Feb 2020 at 13:27, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
>
> From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx>
> Testcase: igt/core_setmaster/master-drop-set-user
> Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
