On 5/27/19 3:16 PM, Daniel Vetter wrote:
On Mon, May 27, 2019 at 02:39:18PM +0200, Thomas Hellstrom wrote:
On 5/27/19 10:17 AM, Emil Velikov wrote:
From: Emil Velikov <emil.veli...@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)
v3:
- Use single unlikely (Daniel V)
v4:
- Patch was reverted because it broke AMDGPU, apply again. The AMDGPU
issue is fixed with earlier patch.
[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-...@lists.freedesktop.org
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Link:
https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.veli...@gmail.com
---
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 9841c0076f02..b64b022a2b29 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -511,6 +511,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
*
@@ -525,14 +532,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 (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))
+ return -EACCES;
+ }
This breaks vmwgfx primary client authentication in the surface_reference
ioctl, which takes different paths in case of render clients and primary
clients, but adding an auth check in the primary path in the vmwgfx code
should fix this.
Hm yeah we need to adjust that ... otoh kinda not sure why this is gated
on authentication status, and not on "am I master or not" status. At least
from a very cursory read ...
-Daniel
The code snippet in question is:
if (drm_is_primary_client(file_priv) &&
user_srf->master != file_priv->master) {
DRM_ERROR("Trying to reference surface outside of"
" master domain.\n");
ret = -EACCES;
goto out_bad_resource;
}
In gem term's this means a client can't open a surface that hasn't been
flinked by a client in the same master realm: You can't read from
resources belonging to another X server's clients....
/Thomas
/Thomas
/* MASTER is only for master or control clients */
if (unlikely((flags & DRM_MASTER) &&
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
