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

From: Emil Velikov <emil.l.velikov@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls
Date: Thu, 20 Dec 2018 15:16:15 +0000	[thread overview]
Message-ID: <CACvgo52VhyPAT2zfzvA9o_JiUtKtkfpsxXLzTL3vnYn4D1jFxA@mail.gmail.com> (raw)
In-Reply-To: <20181219203450.GW21184@phenom.ffwll.local>

On Wed, 19 Dec 2018 at 20:34, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Dec 19, 2018 at 07:22:47PM +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. 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 <emil.velikov@collabora.com>
> > ---
> >  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 <daniel.vetter@ffwll.ch>
>
Smashing thank you. Please let me know if the above information and
links are sufficient.

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