On Thu, Dec 20, 2018 at 03:16:15PM +0000, Emil Velikov wrote: > On Wed, 19 Dec 2018 at 20:34, Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > On Wed, Dec 19, 2018 at 07:22:47PM +0000, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > > > > > > 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 <emil.velikov@xxxxxxxxxxxxx> > > > --- > > > 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 <daniel.vetter@xxxxxxxx> > > > Smashing thank you. Please let me know if the above information and > links are sufficient. Ack. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
