[3/3] drm: allow render capable master with DRM_AUTH ioctls

Commit Message

Emil Velikov Dec. 19, 2018, 7:22 p.m. UTC

From: Emil Velikov <[e-mail]>

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. Mesa has been fixed recently
although, there's the question of older drivers or other apps that
exbibit this behaviour.

To workaround this, 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.

As an added bonus this allows us to use vgem in userspace with zero
change to some (but not all) existing programs.

Signed-off-by: Emil Velikov <[e-mail]>
---
 drivers/gpu/drm/drm_ioctl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Daniel Vetter Dec. 19, 2018, 8:34 p.m. UTC | #1

On Wed, Dec 19, 2018 at 07:22:47PM +0000, Emil Velikov wrote:
> From: Emil Velikov <[e-mail]>
> 
> 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. Mesa has been fixed recently
> although, there's the question of older drivers or other apps that
> exbibit this behaviour.

Would be good to have links to mesa where these bugs are fixed (or
wherever those bugs where).

> 
> To workaround this, 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.
> 
> As an added bonus this allows us to use vgem in userspace with zero
> change to some (but not all) existing programs.

How/what/where?

> Signed-off-by: Emil Velikov <[e-mail]>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 2221c8857fb0..4c775b775395 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -521,13 +521,17 @@ 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 */
> +	/* AUTH is only for authenticated/render capable master or render client */
>  	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
> -		     !file_priv->authenticated))
> +		     !file_priv->authenticated &&
> +		     !(drm_core_check_feature(dev, DRIVER_RENDER) &&
> +		       (flags & DRM_RENDER_ALLOW))))

Gets a bit unreadable but looks correct.

With the commit message improved (since this is new uapi, so needs those
pesky userspace links):

Reviewed-by: Daniel Vetter <[e-mail]>

>  		return -EACCES;
>  
>  	/* MASTER is only for master or control clients */
> -- 
> 2.19.2
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Emil Velikov Dec. 20, 2018, 3:16 p.m. UTC | #2

On Wed, 19 Dec 2018 at 20:34, Daniel Vetter <[e-mail]> wrote:
>
> On Wed, Dec 19, 2018 at 07:22:47PM +0000, Emil Velikov wrote:
> > From: Emil Velikov <[e-mail]>
> >
> > 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. Mesa has been fixed recently
> > although, there's the question of older drivers or other apps that
> > exbibit this behaviour.
>
> Would be good to have links to mesa where these bugs are fixed (or
> wherever those bugs where).
>
The error checking for drmGetMagic() was missing :-\
https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136

> >
> > To workaround this, some users resort to running their apps under sudo.
> > Which admittedly isn't always a good idea.
> >
One sudo example:
https://lists.freedesktop.org/archives/libva/2016-July/004185.html

Note: libva itself doesn't authenticate the DRM client and the
vaGetDisplayDRM documentation doesn't mention if the app should
either.
Fairly neither one does as it - as in the example above.

Typical output when auth handling is missing somewhere in the stack:
https://gitlab.freedesktop.org/mesa/kmscube/issues/1

> > 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.
> >
> > As an added bonus this allows us to use vgem in userspace with zero
> > change to some (but not all) existing programs.
>
> How/what/where?
>
Not quite sure what you mean here. Are you interested in what
userspace will work unmodified with vgem, or something else?

Userspace (such as some IGT vgem tests) that uses dumb buffers,
fences, etc yet lacks drm authentication will work.
Admittedly IGT is the first client (or ran as root), so it's
implicitly authenticated - others may not.

Admittedly, I did not search the webs for explicit vgem examples,
although an educated assumption is that people will take the IGT code
as norm.

> > Signed-off-by: Emil Velikov <[e-mail]>
> > ---
> >  drivers/gpu/drm/drm_ioctl.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 2221c8857fb0..4c775b775395 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -521,13 +521,17 @@ 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 */
> > +     /* AUTH is only for authenticated/render capable master or render client */
> >       if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
> > -                  !file_priv->authenticated))
> > +                  !file_priv->authenticated &&
> > +                  !(drm_core_check_feature(dev, DRIVER_RENDER) &&
> > +                    (flags & DRM_RENDER_ALLOW))))
>
> Gets a bit unreadable but looks correct.
>
Agreed. If you prefer I could add an helper, say:

+static inline bool
+drm_is_render_driver_and_ioctl(...)
+{
+  return drm_core_check_feature(dev, DRIVER_RENDER) && (flags &
DRM_RENDER_ALLOW));
+}

[snip]

-     /* AUTH is only for authenticated or render client */
+     /* AUTH is only for authenticated/render capable master or
render client */
      if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
-                  !file_priv->authenticated))
+                  !file_priv->authenticated &&
!drm_is_render_driver_and_ioctl(...)))

> With the commit message improved (since this is new uapi, so needs those
> pesky userspace links):
>
> Reviewed-by: Daniel Vetter <[e-mail]>
>
Smashing thank you. Please let me know if the above information and
links are sufficient.

-Emil

Daniel Vetter Dec. 20, 2018, 3:34 p.m. UTC | #3

On Thu, Dec 20, 2018 at 03:16:15PM +0000, Emil Velikov wrote:
> On Wed, 19 Dec 2018 at 20:34, Daniel Vetter <[e-mail]> wrote:
> >
> > On Wed, Dec 19, 2018 at 07:22:47PM +0000, Emil Velikov wrote:
> > > From: Emil Velikov <[e-mail]>
> > >
> > > 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. Mesa has been fixed recently
> > > although, there's the question of older drivers or other apps that
> > > exbibit this behaviour.
> >
> > Would be good to have links to mesa where these bugs are fixed (or
> > wherever those bugs where).
> >
> The error checking for drmGetMagic() was missing :-\
> https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136
> 
> > >
> > > To workaround this, some users resort to running their apps under sudo.
> > > Which admittedly isn't always a good idea.
> > >
> One sudo example:
> https://lists.freedesktop.org/archives/libva/2016-July/004185.html
> 
> Note: libva itself doesn't authenticate the DRM client and the
> vaGetDisplayDRM documentation doesn't mention if the app should
> either.
> Fairly neither one does as it - as in the example above.
> 
> Typical output when auth handling is missing somewhere in the stack:
> https://gitlab.freedesktop.org/mesa/kmscube/issues/1

Yup, the above is exactly what I was looking for. I think pasting the
entire thing into the commit message is all we need.
> 
> > > 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.
> > >
> > > As an added bonus this allows us to use vgem in userspace with zero
> > > change to some (but not all) existing programs.
> >
> > How/what/where?
> >
> Not quite sure what you mean here. Are you interested in what
> userspace will work unmodified with vgem, or something else?
> 
> Userspace (such as some IGT vgem tests) that uses dumb buffers,
> fences, etc yet lacks drm authentication will work.
> Admittedly IGT is the first client (or ran as root), so it's
> implicitly authenticated - others may not.

igt isn't really a use-case, there's explicit checks that nothing else is
running and that you're root. The tests don't work otherwise. Some might,
but you can't make that assumption at all.

> Admittedly, I did not search the webs for explicit vgem examples,
> although an educated assumption is that people will take the IGT code
> as norm.

I think clearer to remove this "added bonus" from the commit message.

> > > Signed-off-by: Emil Velikov <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/drm_ioctl.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index 2221c8857fb0..4c775b775395 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -521,13 +521,17 @@ 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 */
> > > +     /* AUTH is only for authenticated/render capable master or render client */
> > >       if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
> > > -                  !file_priv->authenticated))
> > > +                  !file_priv->authenticated &&
> > > +                  !(drm_core_check_feature(dev, DRIVER_RENDER) &&
> > > +                    (flags & DRM_RENDER_ALLOW))))
> >
> > Gets a bit unreadable but looks correct.
> >
> Agreed. If you prefer I could add an helper, say:
> 
> +static inline bool
> +drm_is_render_driver_and_ioctl(...)
> +{
> +  return drm_core_check_feature(dev, DRIVER_RENDER) && (flags &
> DRM_RENDER_ALLOW));
> +}
> 
> [snip]
> 
> -     /* AUTH is only for authenticated or render client */
> +     /* AUTH is only for authenticated/render capable master or
> render client */
>       if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
> -                  !file_priv->authenticated))
> +                  !file_priv->authenticated &&
> !drm_is_render_driver_and_ioctl(...)))

Not sure that's really better, was thinking more of splitting this up into
a set of separate if checks, e.g.

if ((flags & DRM_AUTH) && drm_is_primary_client(file_priv)) {
	if (unlikely(!file_priv->authenticated))
		return -EACCESS;
	if (unlikely(!drm_render_driver_and_ioctl(...))
		return -EACCESS;
}

> > With the commit message improved (since this is new uapi, so needs those
> > pesky userspace links):
> >
> > Reviewed-by: Daniel Vetter <[e-mail]>
> >
> Smashing thank you. Please let me know if the above information and
> links are sufficient.

Ack.
-Daniel

Patch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 2221c8857fb0..4c775b775395 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -521,13 +521,17 @@  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 */
+	/* AUTH is only for authenticated/render capable master or render client */
 	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
-		     !file_priv->authenticated))
+		     !file_priv->authenticated &&
+		     !(drm_core_check_feature(dev, DRIVER_RENDER) &&
+		       (flags & DRM_RENDER_ALLOW))))
 		return -EACCES;
 
 	/* MASTER is only for master or control clients */