diff mbox series

[v2,2/2] drm: allow render capable master with DRM_AUTH ioctls

Message ID 20190114085408.15933-2-emil.l.velikov@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] drm: annotate drm_core_check_feature() dev arg. as const | expand

Commit Message

Emil Velikov Jan. 14, 2019, 8:54 a.m. UTC 
From: Emil Velikov <emil.velikov@collabora.com>

There are cases (in mesa and applications) where one would open the
primary node without properly authenticating the client.

Sometimes we don't check if the authentication succeeds, but there's
also cases we simply forget to do it.

The former was a case for Mesa where it did not not check the return
value of drmGetMagic() [1]. That was fixed recently although, there's
the question of older drivers or other apps that exbibit this behaviour.

While omitting the call results in issues as seen in [2] and [3].

In the libva case, libva itself doesn't authenticate the DRM client and
the vaGetDisplayDRM documentation doesn't mention if the app should
either.

As of today, the official vainfo utility doesn't authenticate.

To workaround issues like these, some users resort to running their apps
under sudo. Which admittedly isn't always a good idea.

Since any DRIVER_RENDER driver has sufficient isolation between clients,
we can use that, for unauthenticated [primary node] ioctls that require
DRM_AUTH. But only if the respective ioctl is tagged as DRM_RENDER_ALLOW.

v2:
- Rework/simplify if check (Daniel V)
- Add examples to commit messages, elaborate. (Daniel V)

[1] https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136
[2] https://lists.freedesktop.org/archives/libva/2016-July/004185.html
[3] https://gitlab.freedesktop.org/mesa/kmscube/issues/1
Testcase: igt/core_unauth_vs_render
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Daniel, the if conditionals did not work exactly as you put them.
This is the closest thing that I can think of.
---
 drivers/gpu/drm/drm_ioctl.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Daniel Vetter Jan. 14, 2019, 10:15 a.m. UTC | #1 
On Mon, Jan 14, 2019 at 08:54:08AM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> There are cases (in mesa and applications) where one would open the
> primary node without properly authenticating the client.
> 
> Sometimes we don't check if the authentication succeeds, but there's
> also cases we simply forget to do it.
> 
> The former was a case for Mesa where it did not not check the return
> value of drmGetMagic() [1]. That was fixed recently although, there's
> the question of older drivers or other apps that exbibit this behaviour.
> 
> While omitting the call results in issues as seen in [2] and [3].
> 
> In the libva case, libva itself doesn't authenticate the DRM client and
> the vaGetDisplayDRM documentation doesn't mention if the app should
> either.
> 
> As of today, the official vainfo utility doesn't authenticate.
> 
> To workaround issues like these, some users resort to running their apps
> under sudo. Which admittedly isn't always a good idea.
> 
> Since any DRIVER_RENDER driver has sufficient isolation between clients,
> we can use that, for unauthenticated [primary node] ioctls that require
> DRM_AUTH. But only if the respective ioctl is tagged as DRM_RENDER_ALLOW.
> 
> v2:
> - Rework/simplify if check (Daniel V)
> - Add examples to commit messages, elaborate. (Daniel V)
> 
> [1] https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136
> [2] https://lists.freedesktop.org/archives/libva/2016-July/004185.html
> [3] https://gitlab.freedesktop.org/mesa/kmscube/issues/1
> Testcase: igt/core_unauth_vs_render
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Daniel, the if conditionals did not work exactly as you put them.
> This is the closest thing that I can think of.
> ---
>  drivers/gpu/drm/drm_ioctl.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 94bd872d56c4..08a0b4cc3a76 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -507,6 +507,13 @@ int drm_version(struct drm_device *dev, void *data,
>  	return err;
>  }
>  
> +static inline bool
> +drm_render_driver_and_ioctl(const struct drm_device *dev, u32 flags)
> +{
> +	return drm_core_check_feature(dev, DRIVER_RENDER) &&
> +		(flags & DRM_RENDER_ALLOW);
> +}
> +
>  /**
>   * drm_ioctl_permit - Check ioctl permissions against caller
>   *
> @@ -521,14 +528,19 @@ int drm_version(struct drm_device *dev, void *data,
>   */
>  int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>  {
> +	const struct drm_device *dev = file_priv->minor->dev;
> +
>  	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
>  	if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
>  		return -EACCES;
>  
> -	/* AUTH is only for authenticated or render client */
> -	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
> -		     !file_priv->authenticated))
> -		return -EACCES;
> +	/* AUTH is only for master ... */
> +	if ((flags & DRM_AUTH) && drm_is_primary_client(file_priv)) {
> +		/* authenticated ones, or render capable on DRM_RENDER_ALLOW. */
> +		if (unlikely(!file_priv->authenticated) &&
> +		    unlikely(!drm_render_driver_and_ioctl(dev, flags)))

The double-unlikely looks a bit strange, I'd move it out so there's only
one. But this is correct too (because unlikely() && unlikely == unlikely(
&& )). Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +			return -EACCES;
> +	}
>  
>  	/* MASTER is only for master or control clients */
>  	if (unlikely((flags & DRM_MASTER) &&
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Emil Velikov Jan. 16, 2019, 11:58 a.m. UTC | #2 
On 2019/01/14, Daniel Vetter wrote:
> On Mon, Jan 14, 2019 at 08:54:08AM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> > 
> > There are cases (in mesa and applications) where one would open the
> > primary node without properly authenticating the client.
> > 
> > Sometimes we don't check if the authentication succeeds, but there's
> > also cases we simply forget to do it.
> > 
> > The former was a case for Mesa where it did not not check the return
> > value of drmGetMagic() [1]. That was fixed recently although, there's
> > the question of older drivers or other apps that exbibit this behaviour.
> > 
> > While omitting the call results in issues as seen in [2] and [3].
> > 
> > In the libva case, libva itself doesn't authenticate the DRM client and
> > the vaGetDisplayDRM documentation doesn't mention if the app should
> > either.
> > 
> > As of today, the official vainfo utility doesn't authenticate.
> > 
> > To workaround issues like these, some users resort to running their apps
> > under sudo. Which admittedly isn't always a good idea.
> > 
> > Since any DRIVER_RENDER driver has sufficient isolation between clients,
> > we can use that, for unauthenticated [primary node] ioctls that require
> > DRM_AUTH. But only if the respective ioctl is tagged as DRM_RENDER_ALLOW.
> > 
> > v2:
> > - Rework/simplify if check (Daniel V)
> > - Add examples to commit messages, elaborate. (Daniel V)
> > 
> > [1] https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136
> > [2] https://lists.freedesktop.org/archives/libva/2016-July/004185.html
> > [3] https://gitlab.freedesktop.org/mesa/kmscube/issues/1
> > Testcase: igt/core_unauth_vs_render
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > Daniel, the if conditionals did not work exactly as you put them.
> > This is the closest thing that I can think of.
> > ---
> >  drivers/gpu/drm/drm_ioctl.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 94bd872d56c4..08a0b4cc3a76 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -507,6 +507,13 @@ int drm_version(struct drm_device *dev, void *data,
> >  	return err;
> >  }
> >  
> > +static inline bool
> > +drm_render_driver_and_ioctl(const struct drm_device *dev, u32 flags)
> > +{
> > +	return drm_core_check_feature(dev, DRIVER_RENDER) &&
> > +		(flags & DRM_RENDER_ALLOW);
> > +}
> > +
> >  /**
> >   * drm_ioctl_permit - Check ioctl permissions against caller
> >   *
> > @@ -521,14 +528,19 @@ int drm_version(struct drm_device *dev, void *data,
> >   */
> >  int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
> >  {
> > +	const struct drm_device *dev = file_priv->minor->dev;
> > +
> >  	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
> >  	if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
> >  		return -EACCES;
> >  
> > -	/* AUTH is only for authenticated or render client */
> > -	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
> > -		     !file_priv->authenticated))
> > -		return -EACCES;
> > +	/* AUTH is only for master ... */
> > +	if ((flags & DRM_AUTH) && drm_is_primary_client(file_priv)) {
> > +		/* authenticated ones, or render capable on DRM_RENDER_ALLOW. */
> > +		if (unlikely(!file_priv->authenticated) &&
> > +		    unlikely(!drm_render_driver_and_ioctl(dev, flags)))
> 
> The double-unlikely looks a bit strange, I'd move it out so there's only
> one. But this is correct too (because unlikely() && unlikely == unlikely(
> && )). Either way:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
Ack, thanks. To stay consistent with the surrounding code I'll use:

	if (unlikely((flags & DRM_AUTH) && drm_is_primary_client(file_priv))) {
		/* authenticated ones, or render capable on DRM_RENDER_ALLOW. */
		if (!file_priv->authenticated &&
		    !drm_render_driver_and_ioctl(dev, flags))



Btw, if you can skim through the IGT test that would be appreciated.

Cheers,
Emil
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 94bd872d56c4..08a0b4cc3a76 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -507,6 +507,13 @@  int drm_version(struct drm_device *dev, void *data,
 	return err;
 }
 
+static inline bool
+drm_render_driver_and_ioctl(const struct drm_device *dev, u32 flags)
+{
+	return drm_core_check_feature(dev, DRIVER_RENDER) &&
+		(flags & DRM_RENDER_ALLOW);
+}
+
 /**
  * drm_ioctl_permit - Check ioctl permissions against caller
  *
@@ -521,14 +528,19 @@  int drm_version(struct drm_device *dev, void *data,
  */
 int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 {
+	const struct drm_device *dev = file_priv->minor->dev;
+
 	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
 	if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
 		return -EACCES;
 
-	/* AUTH is only for authenticated or render client */
-	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
-		     !file_priv->authenticated))
-		return -EACCES;
+	/* AUTH is only for master ... */
+	if ((flags & DRM_AUTH) && drm_is_primary_client(file_priv)) {
+		/* authenticated ones, or render capable on DRM_RENDER_ALLOW. */
+		if (unlikely(!file_priv->authenticated) &&
+		    unlikely(!drm_render_driver_and_ioctl(dev, flags)))
+			return -EACCES;
+	}
 
 	/* MASTER is only for master or control clients */
 	if (unlikely((flags & DRM_MASTER) &&