[PATCH] drm: rework SET_MASTER and DROP_MASTER perm handling

[PATCH] drm: rework SET_MASTER and DROP_MASTER perm handling

[Emil Velikov]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4049 bytes --]

On Wed, 19 Feb 2020 13:27:28 +0000
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> From: Emil Velikov <emil.velikov@collabora.com>
> 

...

> +/*
> + * 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.
> + *

Hi,

sorry I had to trim this email harshly, but Google did not want to
deliver it otherwise.

I agree that being able to drop master without CAP_SYS_ADMIN sounds
like a good thing.

> + *
> + * 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.

I understand the drop master part, because it is needed to get rid of
apps that accidentally gain DRM master because they were the first one
to open the DRM device (on a particular VT?). It could happen e.g. if a
Wayland client is inspecting DRM devices to figure what it wants to
lease while the user has VT-switched to a text-mode VT, I guess. E.g.
starting a Wayland VR compositor from a VT for whatever reason.

The set master without CAP_SYS_ADMIN part I don't understand.

> + *
> + * As a result this fixes, the following when using root-less build w/o logind

Why is non-root without any logind-equivalent a use case that should
work?

Why did DRM set/drop master use to require CAP_SYS_ADMIN in the first
place?

What else happens if we allow DRM set master more than just for
CAP_SYS_ADMIN?

Does this interact with DRM leasing?

Looks like drmIsMaster() should be unaffected at least:
https://patchwork.kernel.org/patch/10776525/

> + * - 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;

In case a helper e.g. logind opens the device, is file_priv->pid then
referring to logind regardless of what happens afterwards?

> +
> +	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);

Why does drop-master need any kind of permission check? Why could it
not be free for all?


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On Fri, 6 Mar 2020 at 14:00, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Wed, 19 Feb 2020 13:27:28 +0000
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
>
> ...
>
> > +/*
> > + * 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.
> > + *
>
> Hi,
>
> sorry I had to trim this email harshly, but Google did not want to
> deliver it otherwise.
>
> I agree that being able to drop master without CAP_SYS_ADMIN sounds
> like a good thing.
>
> > + *
> > + * 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.
>
> I understand the drop master part, because it is needed to get rid of
> apps that accidentally gain DRM master because they were the first one
> to open the DRM device (on a particular VT?). It could happen e.g. if a
> Wayland client is inspecting DRM devices to figure what it wants to
> lease while the user has VT-switched to a text-mode VT, I guess. E.g.
> starting a Wayland VR compositor from a VT for whatever reason.
>
> The set master without CAP_SYS_ADMIN part I don't understand.
>
As you point out application can drop master for various reasons. One
of which may be to say spawn another program which requires master for
_non_ modeset reasons. For example:
 - amdgpu: create a renderer and use the context/process priority override IOCTL
 - vmwgfx: stream claim/unref (amongst others).

Another case to consider is classic X or Wayland compositor. With
CAP_SYS_ADMIN for DROP_MASTER removed, yet retained in SET_MASTER, the
IOCTL will fail. Thus:
 - weston results in frozen session and session switching (have to
force kill weston or sudo loginctl kill-session)
 - depending on the driver, X will work or crash

To make this clearer I'll include //comment sections in the code.

// comment
To ensure the application can reclaim its master status, the tweaked
CAP_SYS_ADMIN handling is needed for both IOCTLs. Otherwise X or
Wayland compositors may freeze or crash as SET_MASTER fails.
// comment


> > + *
> > + * As a result this fixes, the following when using root-less build w/o logind
>
> Why is non-root without any logind-equivalent a use case that should
> work?
>
// comment
Some platforms don't have equivalent (Android, CrOS, some BSDs), yet
root is required _solely_ for DROP/SET MASTER. So tweaking the
requirement sounds perfectly reasonable.
// comment

> Why did DRM set/drop master use to require CAP_SYS_ADMIN in the first
> place?
>
I imagine something else could have been introduced instead. Although
I cannot find any details or discussion on the topic.

> What else happens if we allow DRM set master more than just for
> CAP_SYS_ADMIN?
>
If we're talking about removing CAP_SYS_ADMIN all together:
 - screen scraping by any random application
 - dead trivial way to DoS your compositor


> Does this interact with DRM leasing?
>
> Looks like drmIsMaster() should be unaffected at least:
> https://patchwork.kernel.org/patch/10776525/
>
Correct, both are unaffected. All the leasing IOCTLs happen by the
active true (aka non-lease) master.


> > + * - 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;
>
> In case a helper e.g. logind opens the device, is file_priv->pid then
> referring to logind regardless of what happens afterwards?
>
Correct.

> > +
> > +     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);
>
> Why does drop-master need any kind of permission check? Why could it
> not be free for all?
>
Consider the arbitrator usecase - be that logind, Xorg (in ancient
times) or otherwise.

// comment
DROP_MASTER cannot be free for all, as any (say logind) user can:
 - can DoS/crash the arbitrator
 - open the node, become master implicitly and cause issues
// comment

I've added an IGT subtest to ensure this does not happen.

Let me know if I should include anything more to the commit, than the
above comment sections.

Thanks

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 8652 bytes --]

On Fri, 6 Mar 2020 18:51:22 +0000
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> On Fri, 6 Mar 2020 at 14:00, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Wed, 19 Feb 2020 13:27:28 +0000
> > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >  
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > >  
> >
> > ...
> >  
> > > +/*
> > > + * 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.
> > > + *  
> >
> > Hi,
> >
> > sorry I had to trim this email harshly, but Google did not want to
> > deliver it otherwise.
> >
> > I agree that being able to drop master without CAP_SYS_ADMIN sounds
> > like a good thing.
> >  
> > > + *
> > > + * 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.  
> >
> > I understand the drop master part, because it is needed to get rid of
> > apps that accidentally gain DRM master because they were the first one
> > to open the DRM device (on a particular VT?). It could happen e.g. if a
> > Wayland client is inspecting DRM devices to figure what it wants to
> > lease while the user has VT-switched to a text-mode VT, I guess. E.g.
> > starting a Wayland VR compositor from a VT for whatever reason.
> >
> > The set master without CAP_SYS_ADMIN part I don't understand.
> >  
> As you point out application can drop master for various reasons. One
> of which may be to say spawn another program which requires master for
> _non_ modeset reasons. For example:
>  - amdgpu: create a renderer and use the context/process priority override IOCTL
>  - vmwgfx: stream claim/unref (amongst others).

Hi,

if none of that is about KMS resources specifically, then to me it
seems like a mis-design that should be avoided if still possible. DRM
master is all about modeset, and in my option should be about nothing
else.

Are those needed to keep existing userspace working?


> Another case to consider is classic X or Wayland compositor. With
> CAP_SYS_ADMIN for DROP_MASTER removed, yet retained in SET_MASTER, the
> IOCTL will fail. Thus:
>  - weston results in frozen session and session switching (have to
> force kill weston or sudo loginctl kill-session)
>  - depending on the driver, X will work or crash
> 
> To make this clearer I'll include //comment sections in the code.
> 
> // comment
> To ensure the application can reclaim its master status, the tweaked
> CAP_SYS_ADMIN handling is needed for both IOCTLs. Otherwise X or
> Wayland compositors may freeze or crash as SET_MASTER fails.
> // comment

A Wayland compositor or Xorg that got DRM master by first-opener up to
now has not been able to drop DRM master, which means VT switching away
does not work - does it? If drop-master succeeded, then VT-switch back
doesn't work, which in my opinion is VT-switching failing again just in
a different way.

OTOH, if applications exist that rely on drop-master failing in this
specific case, making drop-master succeed would break them. That might
include a buggy set-master path that was written, but does not actually
work because it was never tested since drop-master never worked.

So I do not fully buy this argument yet, but I also cannot name any
explicit examples that would break.


> > > + *
> > > + * As a result this fixes, the following when using root-less build w/o logind  
> >
> > Why is non-root without any logind-equivalent a use case that should
> > work?
> >  
> // comment
> Some platforms don't have equivalent (Android, CrOS, some BSDs), yet
> root is required _solely_ for DROP/SET MASTER. So tweaking the
> requirement sounds perfectly reasonable.
> // comment

For those that use a Linux kernel, I disagree. For those that do not
use a Linux kernel, how is this relevant?


> > Why did DRM set/drop master use to require CAP_SYS_ADMIN in the first
> > place?
> >  
> I imagine something else could have been introduced instead. Although
> I cannot find any details or discussion on the topic.
> 
> > What else happens if we allow DRM set master more than just for
> > CAP_SYS_ADMIN?
> >  
> If we're talking about removing CAP_SYS_ADMIN all together:
>  - screen scraping by any random application
>  - dead trivial way to DoS your compositor

No, I am asking about your specific proposal here. This is the question
about which neither of us can see more than we already wrote, so it
needs someone else to think hard.


> > Does this interact with DRM leasing?
> >
> > Looks like drmIsMaster() should be unaffected at least:
> > https://patchwork.kernel.org/patch/10776525/
> >  
> Correct, both are unaffected. All the leasing IOCTLs happen by the
> active true (aka non-lease) master.

Do you assume that DRM leasing clients (lessee) will not or cannot call
set-master/drop-master ioctls?

What happens if they do call set/drop master ioctls on a leased DRM file
description?

After all, the leased DRM file description must be good for modeset
operations that normally need the real DRM master.


> > > + * - 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;  
> >
> > In case a helper e.g. logind opens the device, is file_priv->pid then
> > referring to logind regardless of what happens afterwards?
> >  
> Correct.
> 
> > > +
> > > +     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);  
> >
> > Why does drop-master need any kind of permission check? Why could it
> > not be free for all?
> >  
> Consider the arbitrator usecase - be that logind, Xorg (in ancient
> times) or otherwise.
> 
> // comment
> DROP_MASTER cannot be free for all, as any (say logind) user can:
>  - can DoS/crash the arbitrator

How would this happen?


>  - open the node, become master implicitly and cause issues

How would this follow? Is this not already the case and also remain the
case even with your changes?


Thanks,
pq

> // comment
> 
> I've added an IGT subtest to ensure this does not happen.
> 
> Let me know if I should include anything more to the commit, than the
> above comment sections.
> 
> Thanks
> 
> -Emil


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On Mon, 9 Mar 2020 at 08:38, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Fri, 6 Mar 2020 18:51:22 +0000
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> > On Fri, 6 Mar 2020 at 14:00, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >
> > > On Wed, 19 Feb 2020 13:27:28 +0000
> > > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > >
> > >
> > > ...
> > >
> > > > +/*
> > > > + * 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.
> > > > + *
> > >
> > > Hi,
> > >
> > > sorry I had to trim this email harshly, but Google did not want to
> > > deliver it otherwise.
> > >
> > > I agree that being able to drop master without CAP_SYS_ADMIN sounds
> > > like a good thing.
> > >
> > > > + *
> > > > + * 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.
> > >
> > > I understand the drop master part, because it is needed to get rid of
> > > apps that accidentally gain DRM master because they were the first one
> > > to open the DRM device (on a particular VT?). It could happen e.g. if a
> > > Wayland client is inspecting DRM devices to figure what it wants to
> > > lease while the user has VT-switched to a text-mode VT, I guess. E.g.
> > > starting a Wayland VR compositor from a VT for whatever reason.
> > >
> > > The set master without CAP_SYS_ADMIN part I don't understand.
> > >
> > As you point out application can drop master for various reasons. One
> > of which may be to say spawn another program which requires master for
> > _non_ modeset reasons. For example:
> >  - amdgpu: create a renderer and use the context/process priority override IOCTL
> >  - vmwgfx: stream claim/unref (amongst others).
>
> Hi,
>
> if none of that is about KMS resources specifically, then to me it
> seems like a mis-design that should be avoided if still possible. DRM
> master is all about modeset, and in my option should be about nothing
> else.
>
> Are those needed to keep existing userspace working?
>
In a perfect world sure. In practical terms - fixing these is less
likely. We have limited time and thou shalt not regress userspace.
About specifics on the individual drivers, I'll refer you to the
respective teams.

>
> > Another case to consider is classic X or Wayland compositor. With
> > CAP_SYS_ADMIN for DROP_MASTER removed, yet retained in SET_MASTER, the
> > IOCTL will fail. Thus:
> >  - weston results in frozen session and session switching (have to
> > force kill weston or sudo loginctl kill-session)
> >  - depending on the driver, X will work or crash
> >
> > To make this clearer I'll include //comment sections in the code.
> >
> > // comment
> > To ensure the application can reclaim its master status, the tweaked
> > CAP_SYS_ADMIN handling is needed for both IOCTLs. Otherwise X or
> > Wayland compositors may freeze or crash as SET_MASTER fails.
> > // comment
>
> A Wayland compositor or Xorg that got DRM master by first-opener up to
> now has not been able to drop DRM master, which means VT switching away
> does not work - does it?
Correct - it does not. Yet the return code of the drmDropMaster is
ignored so the compositor continues working... Somewhat. Weston got
some checks recently IIRC.

> If drop-master succeeded, then VT-switch back
> doesn't work, which in my opinion is VT-switching failing again just in
> a different way.
>
If the drmSetMaster fails (while Drop was successful), you'll get
user-facing issues.
The crash or freeze as mentioned earlier.

> OTOH, if applications exist that rely on drop-master failing in this
> specific case, making drop-master succeed would break them. That might
> include a buggy set-master path that was written, but does not actually
> work because it was never tested since drop-master never worked.
>
> So I do not fully buy this argument yet, but I also cannot name any
> explicit examples that would break.
>
>
I've ventured for a while in the X (Xorg + drivers), Weston,
sway/wlroots and Mesa's codebase.
There were zero instances of such misuse. If other projects come to
mind - I'll gladly take a look.

Based on my observation and the drmDropMaster semantics, inclined to
say the example is purely theoretical.
I'm open to be proven otherwise.

> > > > + *
> > > > + * As a result this fixes, the following when using root-less build w/o logind
> > >
> > > Why is non-root without any logind-equivalent a use case that should
> > > work?
> > >
> > // comment
> > Some platforms don't have equivalent (Android, CrOS, some BSDs), yet
> > root is required _solely_ for DROP/SET MASTER. So tweaking the
> > requirement sounds perfectly reasonable.
> > // comment
>
> For those that use a Linux kernel, I disagree. For those that do not
> use a Linux kernel, how is this relevant?
>
Please elaborate.

Both Android and CrOS use the linux kernel. Because of this issue,
they resort to other hacks or workarounds. There are also
distributions which do not ship logind.

I can see how tempting it is to close our eyes and say "not my
problem", although it's not really respectful IMHO.

>
> > > Why did DRM set/drop master use to require CAP_SYS_ADMIN in the first
> > > place?
> > >
> > I imagine something else could have been introduced instead. Although
> > I cannot find any details or discussion on the topic.
> >
> > > What else happens if we allow DRM set master more than just for
> > > CAP_SYS_ADMIN?
> > >
> > If we're talking about removing CAP_SYS_ADMIN all together:
> >  - screen scraping by any random application
> >  - dead trivial way to DoS your compositor
>
> No, I am asking about your specific proposal here. This is the question
> about which neither of us can see more than we already wrote, so it
> needs someone else to think hard.
>
I see what you mean.

I've explored for potential issues for quite some time and discussed
those with couple of people already.
To our mutual understanding there are no implications or issues that
this patch may cause.

>
> > > Does this interact with DRM leasing?
> > >
> > > Looks like drmIsMaster() should be unaffected at least:
> > > https://patchwork.kernel.org/patch/10776525/
> > >
> > Correct, both are unaffected. All the leasing IOCTLs happen by the
> > active true (aka non-lease) master.
>
> Do you assume that DRM leasing clients (lessee) will not or cannot call
> set-master/drop-master ioctls?
>
> What happens if they do call set/drop master ioctls on a leased DRM file
> description?
>
> After all, the leased DRM file description must be good for modeset
> operations that normally need the real DRM master.
>
The leasing infra acts _internally_ as pseudo master. The whole
set/drop master machinery is a no-op for them.

>
> > > > + * - 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;
> > >
> > > In case a helper e.g. logind opens the device, is file_priv->pid then
> > > referring to logind regardless of what happens afterwards?
> > >
> > Correct.
> >
> > > > +
> > > > +     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);
> > >
> > > Why does drop-master need any kind of permission check? Why could it
> > > not be free for all?
> > >
> > Consider the arbitrator usecase - be that logind, Xorg (in ancient
> > times) or otherwise.
> >
> > // comment
> > DROP_MASTER cannot be free for all, as any (say logind) user can:
> >  - can DoS/crash the arbitrator
>
> How would this happen?
>
Specifics depend on the arbitrator - I anticipate a use-after-free
being possible. Looking at logind - it seems safe.

Although logind also ignores the return value of drmDropMaster. Hence
a good example where relaxing DROP_MASTER yet, keeping SET_MASTER
as-is is a bad idea.

>
> >  - open the node, become master implicitly and cause issues
>
> How would this follow? Is this not already the case and also remain the
> case even with your changes?
>
Theoretically yes, although a _lot_ harder to pull on in practise.

Today the arbitrator implicitly does the DROP_MASTER on VT switch et
al. The app has to race between the DROP and the VT switch ACK (also
done by the arbitrator).

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On Mon, 9 Mar 2020 at 13:13, Emil Velikov <emil.l.velikov@gmail.com> wrote:

> > OTOH, if applications exist that rely on drop-master failing in this
> > specific case, making drop-master succeed would break them. That might
> > include a buggy set-master path that was written, but does not actually
> > work because it was never tested since drop-master never worked.
> >
> > So I do not fully buy this argument yet, but I also cannot name any
> > explicit examples that would break.
> >
> >
> I've ventured for a while in the X (Xorg + drivers), Weston,
> sway/wlroots and Mesa's codebase.
> There were zero instances of such misuse. If other projects come to
> mind - I'll gladly take a look.
>
Just checked a few other projects with git pickaxe* - zero cases of
mentioned (mis)use. In particular:
 - qtbase + qtwayland + gtk
Never used the wrappers or ioctls

 - kwin + plasmashell
Never used the wrappers or ioctls

 - mutter + gnome-shell
Briefly used the wrappers. Sane codepath

 - igt-gpu-tools ... just because I had it open
Sane use both wrappers and ioctls.

Any other projects I should check?

-Emil

* Both via libdrm and directly calling the ioctl
git log -p -S DROP_MASTER
git log -p -S drmDropMaster
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On Mon, 9 Mar 2020 at 18:36, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Mon, 9 Mar 2020 at 13:13, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> > > OTOH, if applications exist that rely on drop-master failing in this
> > > specific case, making drop-master succeed would break them. That might
> > > include a buggy set-master path that was written, but does not actually
> > > work because it was never tested since drop-master never worked.
> > >
> > > So I do not fully buy this argument yet, but I also cannot name any
> > > explicit examples that would break.
> > >
> > >
> > I've ventured for a while in the X (Xorg + drivers), Weston,
> > sway/wlroots and Mesa's codebase.
> > There were zero instances of such misuse. If other projects come to
> > mind - I'll gladly take a look.
> >
> Just checked a few other projects with git pickaxe* - zero cases of
> mentioned (mis)use. In particular:
>  - qtbase + qtwayland + gtk
> Never used the wrappers or ioctls
>
>  - kwin + plasmashell
> Never used the wrappers or ioctls
>
>  - mutter + gnome-shell
> Briefly used the wrappers. Sane codepath
>
>  - igt-gpu-tools ... just because I had it open
> Sane use both wrappers and ioctls.
>
> Any other projects I should check?
>
Coming back from an interesting venture in efl world:
 - enlightment - correctly usage during 2012-2014, switched to ecore_drm
 - evas (imlib2 successor) correct usage, few months in 2014, switched
to ecore_drm
 - legacy/ecore - never used either
 - ecore - ecore_drm backend, correct usage of drmSet/DropMaster

Given the above, it does seem that the raised concern is more or less
hypothetical.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On Mon, 2 Mar 2020 at 18:29, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> 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?
>
Another humble poke?

Daniel you seemed on the fence for the RFC.
With the questions raised by Pekka and addressed by yours truly, can
you please review this patch?

The IGT tests have been in the i915-CI for a while now.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Mar 17, 2020 at 1:26 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Mon, 2 Mar 2020 at 18:29, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > 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?
> >
> Another humble poke?
>
> Daniel you seemed on the fence for the RFC.
> With the questions raised by Pekka and addressed by yours truly, can
> you please review this patch?

Just wanted to make sure the igt are address and things look
reasonable for uapi. Jesse just pinged me on irc that CrOS wants
this/needs this, I think best if one of the cros people (we should
have a bunch here) reviews this and then you can push it.

> The IGT tests have been in the i915-CI for a while now.

Yeah thanks a lot for doing that.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Adam Jackson]

On Wed, 2020-02-19 at 13:27 +0000, Emil Velikov wrote:

> + * 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.

I'm not super worried. Everything about VTs is a pile of DoS scenarios
that userspace has to dance to avoid. It sounds like this is only
introducing new DoS scenarios for cases that previously simply did not
work.

> + * As a result this fixes, the following when using root-less build w/o logind

Nitpick: no comma here.

>  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;

I'd mentioned this on IRC, and it doesn't need to be changed with this
patch, but it would be cool if the "does the device already have a
master" check just below here would return -EBUSY instead of -EINVAL so
userspace diagnostics have a chance of saying something useful. A quick
audit of the X drivers and weston shows no cases where we care about
the generated errno value beyond feeding it into strerror() so that
should also be safe.

Looks good otherwise, and these are definitely more reasonable
semantics. Thanks for taking this on!

Reviewed-by: Adam Jackson <ajax@redhat.com>

- ajax

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel