On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov 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 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@xxxxxxxxxx> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > --- > 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 > + */ I think this breaks logind security. With logind no compositor can open the device node directly, hence no compositor can accidentally become the master and block everyone else. And for the vt switch logind is the only one that can grant master rights, and it can make sure that the right compositor gets them. And if the old compositor is non-cooperating, it can also forcefully remove master rights. But with this here we lift this restriction if a compositor has ever been master. So the following thing could happen: - We have 3 compositors for different users C1, C2, C3 - We switch from C1 to C2 - While we switch no one is master for a moment, which means C3 could sneak in and do a quick setmaster, and become master - Everything would come crashing done since logind believes it already revoked master for C1, but somehow it now cant grant master to C2 I'm not sure we can even support these two models at the same time. > +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) Isn't this a typo? Why should we only allow this if the opener is someone else ... that looks like the logind approach? Or is my bolean logic parser broken again. Cheers, Daniel > + 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: > * > -- > 2.25.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
