[RFC] drm: rework SET_MASTER and DROP_MASTER perm handling

Commit Message

Emil Velikov Feb. 5, 2020, 5:48 p.m. UTC

From: Emil Velikov <[e-mail]>

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 <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Signed-off-by: Emil Velikov <[e-mail]>
---
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(-)

Comments

Daniel Vetter Feb. 7, 2020, 1:29 p.m. UTC | #1

On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> From: Emil Velikov <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Signed-off-by: Emil Velikov <[e-mail]>
> ---
> 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
>

Emil Velikov Feb. 10, 2020, 7:01 p.m. UTC | #2

Thanks for having a look Daniel.

On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <[e-mail]> wrote:
>
> On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> > From: Emil Velikov <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > Signed-off-by: Emil Velikov <[e-mail]>
> > ---
> > 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.
>
I've explicitly considered this case. AFAICT this patch does not
change any of the contract.
If you think there's a scenario where things have broken, please let me know.

> 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.
>
Yes logind does set/drop master on VT switch, session setup/teardown, etc.

To take this a step further, there is no logind API or dbus method for
compositors to only set/drop master.
Thus logind ensures that compositors are in sane state.

> 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
>
Does this scenario consider that all three compositors are logind users?
If so, C3 should not be able to set or drop master. Since it got its
fd from logind:

 - `file_priv->pid` will point to systemd-logind, and
 - `task_pid(current)` will point to the respective compositor

-> EACCES will be returned to any compositor calling drmSetMaster.

Regardless of my patch, C3 can open() and simply not release the master.
Assuming it's the first DRM client of course - say switch to VTx +
login + start C3.

I've been lucky enough to spot various ways to softlock my system...
even when the compositor is using logind ;-)
If you're really interested I can share, but I'm worried that people
may see them as bashing at logind.

> 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.
>
Thanks for spotting it. Indeed that should be:

if (file_priv->pid == task_pid(current) && file_priv->was_master)
    return 0;


Modulo any objections, I'll do proper testing and submit a non RFC version.
The inline comments will explicitly mention your concerns and why the
patch is safe.


Thanks
Emil

Daniel Vetter Feb. 10, 2020, 9:54 p.m. UTC | #3

On Mon, Feb 10, 2020 at 8:01 PM Emil Velikov <[e-mail]> wrote:
>
> Thanks for having a look Daniel.
>
> On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <[e-mail]> wrote:
> >
> > On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> > > From: Emil Velikov <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Cc: Daniel Vetter <[e-mail]>
> > > Signed-off-by: Emil Velikov <[e-mail]>
> > > ---
> > > 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.
> >
> I've explicitly considered this case. AFAICT this patch does not
> change any of the contract.
> If you think there's a scenario where things have broken, please let me know.
>
> > 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.
> >
> Yes logind does set/drop master on VT switch, session setup/teardown, etc.
>
> To take this a step further, there is no logind API or dbus method for
> compositors to only set/drop master.
> Thus logind ensures that compositors are in sane state.
>
> > 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
> >
> Does this scenario consider that all three compositors are logind users?
> If so, C3 should not be able to set or drop master. Since it got its
> fd from logind:
>
>  - `file_priv->pid` will point to systemd-logind, and
>  - `task_pid(current)` will point to the respective compositor
>
> -> EACCES will be returned to any compositor calling drmSetMaster.
>
> Regardless of my patch, C3 can open() and simply not release the master.
> Assuming it's the first DRM client of course - say switch to VTx +
> login + start C3.

Hm ... I guess this works indeed. Or should. I'm mildly freaked out
that we're checking for opener_pid == current->pid. Not sure how many
other security assumptions we're breaking.

> I've been lucky enough to spot various ways to softlock my system...
> even when the compositor is using logind ;-)
> If you're really interested I can share, but I'm worried that people
> may see them as bashing at logind.
>
> > 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.
> >
> Thanks for spotting it. Indeed that should be:
>
> if (file_priv->pid == task_pid(current) && file_priv->was_master)
>     return 0;
>
>
> Modulo any objections, I'll do proper testing and submit a non RFC version.
> The inline comments will explicitly mention your concerns and why the
> patch is safe.

Given the above bug I think a solid igt for both the logind and the
non-logind scenario is needed. We have some helpers to drop root and
fork stuff and all that, so shouldn't be many lines.
-Daniel

Pekka Paalanen Feb. 11, 2020, 8:06 a.m. UTC | #4

On Mon, 10 Feb 2020 19:01:06 +0000
Emil Velikov <[e-mail]> wrote:

> Thanks for having a look Daniel.
> 
> On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <[e-mail]> wrote:
> >
> > On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:  
> > > From: Emil Velikov <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Cc: Daniel Vetter <[e-mail]>
> > > Signed-off-by: Emil Velikov <[e-mail]>
> > > ---
> > > 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

> > > +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.
> >  
> Thanks for spotting it. Indeed that should be:
> 
> if (file_priv->pid == task_pid(current) && file_priv->was_master)
>     return 0;

Hi,

I'm mostly just curious, why is comparing pids safe here? Maybe the
'pid' member is not what userspace calls PID?

What if a malicious process receives a DRM fd from something similar to
logind, then the logind equivalent process dies, and the malicious
process starts forking new processes attempting to hit the same pid the
logind equivalent had, succeeds in that, and passes the DRM fd to that
fork. Is the fork then effectively in control of DRM master?


Thanks,
pq

Emil Velikov Feb. 11, 2020, 11:46 a.m. UTC | #5

On Tue, 11 Feb 2020 at 08:06, Pekka Paalanen <[e-mail]> wrote:
>
> On Mon, 10 Feb 2020 19:01:06 +0000
> Emil Velikov <[e-mail]> wrote:
>
> > Thanks for having a look Daniel.
> >
> > On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <[e-mail]> wrote:
> > >
> > > On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> > > > From: Emil Velikov <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Cc: Daniel Vetter <[e-mail]>
> > > > Signed-off-by: Emil Velikov <[e-mail]>
> > > > ---
> > > > 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
>
> > > > +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.
> > >
> > Thanks for spotting it. Indeed that should be:
> >
> > if (file_priv->pid == task_pid(current) && file_priv->was_master)
> >     return 0;
>
> Hi,
>
> I'm mostly just curious, why is comparing pids safe here? Maybe the
> 'pid' member is not what userspace calls PID?
>
PID here is the kernel struct pid. For userspace ones we have the
distinct task_xid_nr, task_xid_vnr and task_xid_nr_ns.
See the documentation [1] for details.

> What if a malicious process receives a DRM fd from something similar to
> logind, then the logind equivalent process dies,
In the logind case, systemd ensures to bring it back up ASAP. For
others - shrug?

> and the malicious
> process starts forking new processes attempting to hit the same pid the
> logind equivalent had, succeeds in that, and passes the DRM fd to that
> fork. Is the fork then effectively in control of DRM master?
>
Valid point, although I believe we're covered.

First and foremost, the pid we store is refcounted [1]. So in order
for this to happen we need have both a) a pretty fundamental refcount
bug for the pid to gets destroyed and b) we need to allocate another
one at the exact same address.

Individually - pretty unlikely, combined - beyond paranoid IMHO.

Additionally, today there are other ways to cause issues. In particular:
 - logind dies
 - the application already has an fd (from logind)
 - the fd is master capable
 - application is free to do as it wishes ... apart from dropping
master (oh noo) and setting it back up again

Or a simple application which loops over open() + drmIsMaster() + close().
There are others, although I'd be going pretty much off-topic.

Thanks
Emil

[1] https://elixir.bootlin.com/linux/v5.5/source/include/linux/sched.h#L1307
[2] https://elixir.bootlin.com/linux/v5.5/source/drivers/gpu/drm/drm_file.c#L127

Emil Velikov Feb. 11, 2020, 11:54 a.m. UTC | #6

On Mon, 10 Feb 2020 at 21:54, Daniel Vetter <[e-mail]> wrote:
>
> On Mon, Feb 10, 2020 at 8:01 PM Emil Velikov <[e-mail]> wrote:
> >
> > Thanks for having a look Daniel.
> >
> > On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <[e-mail]> wrote:
> > >
> > > On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> > > > From: Emil Velikov <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Cc: Daniel Vetter <[e-mail]>
> > > > Signed-off-by: Emil Velikov <[e-mail]>
> > > > ---
> > > > 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.
> > >
> > I've explicitly considered this case. AFAICT this patch does not
> > change any of the contract.
> > If you think there's a scenario where things have broken, please let me know.
> >
> > > 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.
> > >
> > Yes logind does set/drop master on VT switch, session setup/teardown, etc.
> >
> > To take this a step further, there is no logind API or dbus method for
> > compositors to only set/drop master.
> > Thus logind ensures that compositors are in sane state.
> >
> > > 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
> > >
> > Does this scenario consider that all three compositors are logind users?
> > If so, C3 should not be able to set or drop master. Since it got its
> > fd from logind:
> >
> >  - `file_priv->pid` will point to systemd-logind, and
> >  - `task_pid(current)` will point to the respective compositor
> >
> > -> EACCES will be returned to any compositor calling drmSetMaster.
> >
> > Regardless of my patch, C3 can open() and simply not release the master.
> > Assuming it's the first DRM client of course - say switch to VTx +
> > login + start C3.
>
> Hm ... I guess this works indeed. Or should. I'm mildly freaked out
> that we're checking for opener_pid == current->pid. Not sure how many
> other security assumptions we're breaking.
>
Today there are many ways to DoS your compositor - regardless if the
app uses logind or not.
As said in the other thread - we do refcount the pid. So its lifetime
should be preserved and the check will be fine.

> > I've been lucky enough to spot various ways to softlock my system...
> > even when the compositor is using logind ;-)
> > If you're really interested I can share, but I'm worried that people
> > may see them as bashing at logind.
> >
> > > 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.
> > >
> > Thanks for spotting it. Indeed that should be:
> >
> > if (file_priv->pid == task_pid(current) && file_priv->was_master)
> >     return 0;
> >
> >
> > Modulo any objections, I'll do proper testing and submit a non RFC version.
> > The inline comments will explicitly mention your concerns and why the
> > patch is safe.
>
> Given the above bug I think a solid igt for both the logind and the
> non-logind scenario is needed. We have some helpers to drop root and
> fork stuff and all that, so shouldn't be many lines.

The non-logind case would be pretty trivial. The logind one will take
a bit of time - I need to catch up on all the fancy helpers.

Thanks
Emil

Daniel Vetter Feb. 11, 2020, 3:43 p.m. UTC | #7

On Tue, Feb 11, 2020 at 11:46:26AM +0000, Emil Velikov wrote:
> On Tue, 11 Feb 2020 at 08:06, Pekka Paalanen <[e-mail]> wrote:
> >
> > On Mon, 10 Feb 2020 19:01:06 +0000
> > Emil Velikov <[e-mail]> wrote:
> >
> > > Thanks for having a look Daniel.
> > >
> > > On Fri, 7 Feb 2020 at 13:29, Daniel Vetter <[e-mail]> wrote:
> > > >
> > > > On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
> > > > > From: Emil Velikov <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > Cc: Daniel Vetter <[e-mail]>
> > > > > Signed-off-by: Emil Velikov <[e-mail]>
> > > > > ---
> > > > > 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
> >
> > > > > +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.
> > > >
> > > Thanks for spotting it. Indeed that should be:
> > >
> > > if (file_priv->pid == task_pid(current) && file_priv->was_master)
> > >     return 0;
> >
> > Hi,
> >
> > I'm mostly just curious, why is comparing pids safe here? Maybe the
> > 'pid' member is not what userspace calls PID?
> >
> PID here is the kernel struct pid. For userspace ones we have the
> distinct task_xid_nr, task_xid_vnr and task_xid_nr_ns.
> See the documentation [1] for details.
> 
> > What if a malicious process receives a DRM fd from something similar to
> > logind, then the logind equivalent process dies,
> In the logind case, systemd ensures to bring it back up ASAP. For
> others - shrug?
> 
> > and the malicious
> > process starts forking new processes attempting to hit the same pid the
> > logind equivalent had, succeeds in that, and passes the DRM fd to that
> > fork. Is the fork then effectively in control of DRM master?
> >
> Valid point, although I believe we're covered.

Yeah, the kernel-internal pid structure maps to the shiny new pidfd stuff,
not to traditional unix pid numbers with all their problems around races
and reuse when there's not a parent/child relationship.
-Daniel

> 
> First and foremost, the pid we store is refcounted [1]. So in order
> for this to happen we need have both a) a pretty fundamental refcount
> bug for the pid to gets destroyed and b) we need to allocate another
> one at the exact same address.
> 
> Individually - pretty unlikely, combined - beyond paranoid IMHO.
> 
> Additionally, today there are other ways to cause issues. In particular:
>  - logind dies
>  - the application already has an fd (from logind)
>  - the fd is master capable
>  - application is free to do as it wishes ... apart from dropping
> master (oh noo) and setting it back up again
> 
> Or a simple application which loops over open() + drmIsMaster() + close().
> There are others, although I'd be going pretty much off-topic.
> 
> Thanks
> Emil
> 
> [1] https://elixir.bootlin.com/linux/v5.5/source/include/linux/sched.h#L1307
> [2] https://elixir.bootlin.com/linux/v5.5/source/drivers/gpu/drm/drm_file.c#L127

Patch

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:
 	 *