From: Emil Velikov emil.velikov@collabora.com
With later commit we'll rework DRM core authentication handling.
Namely unauthenticated master will be allowed with, DRM_AUTH ioctls. Since vmwgfx does additional master locking and DRM_AUTH handling, this will not matter almost all cases.
The only exception being using the legacy handle type in the family of surface_reference iocts - all handled by vmw_surface_handle_reference(). Add the check to ensure such clients do not access more than they should
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com --- I'd like to merge this through the drm-misc tree. Ack and rb are appreciated.
Thanks Emil
Unrelated: worth moving the is_render_client check alongside the is_primary_client one. --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 219471903bc1..1f5146c95785 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -940,6 +940,13 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, user_srf = container_of(base, struct vmw_user_surface, prime.base);
+ /* Error out if we are unauthenticated master */ + if (drm_is_primary_client(file_priv) && + !file_priv->authenticated) { + ret = -EACCES; + goto out_bad_resource; + } + /* * Make sure the surface creator has the same * authenticating master, or is already registered with us.
From: Emil Velikov emil.velikov@collabora.com
With earlier commits we've removed DRM_AUTH for driver ioctls annotated with DRM_AUTH | DRM_RENDER_ALLOW, as the protection it introduces is effectively not existent.
With next commit, we'll effectively do the same for DRM core.
Yet the AMD developers have voiced concerns that by doing so, developers working on the closed source user-space driver might remove render node support.
Since we do _not_ want that to happen, add workaround for those two drivers
Cc: Alex Deucher alexander.deucher@amd.com Cc: Christian König christian.koenig@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com --- Christian, Alex this is the cleaner way to handle AMDGPU/radeon although if you prefer alternative methods let me know.
Review, acks and others are appreciated, since I'd like to get this through the drm-misc tree.
Thanks Emil
Unrelated: The USE_AGP flag in AMDGPU should be nuked. While for radeon, one can copy in the driver the 10-20 lines worth of agp_init/release and also drop the flag.
Bonus points of agp_init code gets a LEGACY check alongside the USE_AGP one. --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- drivers/gpu/drm/radeon/radeon_drv.c | 2 +- include/drm/drm_drv.h | 10 ++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 8e1b269351e8..cfc2ef11330c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1307,7 +1307,7 @@ amdgpu_get_crtc_scanout_position(struct drm_device *dev, unsigned int pipe,
static struct drm_driver kms_driver = { .driver_features = - DRIVER_USE_AGP | DRIVER_ATOMIC | + DRIVER_USE_AGP | DRIVER_ATOMIC | DRIVER_FORCE_AUTH | DRIVER_GEM | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ, .load = amdgpu_driver_load_kms, diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 4403e76e1ae0..5a1bfad1ad5e 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -538,7 +538,7 @@ radeon_get_crtc_scanout_position(struct drm_device *dev, unsigned int pipe,
static struct drm_driver kms_driver = { .driver_features = - DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER, + DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER | DRIVER_FORCE_AUTH, .load = radeon_driver_load_kms, .open = radeon_driver_open_kms, .postclose = radeon_driver_postclose_kms, diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index b33f2cee2099..5fb2846396bc 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -92,6 +92,16 @@ enum drm_driver_feature { * synchronization of command submission. */ DRIVER_SYNCOBJ_TIMELINE = BIT(6), + /** + * @DRIVER_FORCE_AUTH: + * + * Driver mandates that DRM_AUTH is honoured, even if the same ioctl + * is exposed via the render node - aka any of an "authentication" is + * a fallacy. + * + * Used only by amdgpu and radeon. Do not use. + */ + DRIVER_FORCE_AUTH = BIT(7),
/* IMPORTANT: Below are all the legacy flags, add new ones above. */
Well this is still a NAK.
As stated previously please just don't remove DRM_AUTH and keep the functionality as it is.
I absolutely don't see the point to add a new flag to remove the same functionality a different flag provides.
Christian.
Am 03.07.2019 15:30 schrieb Emil Velikov emil.l.velikov@gmail.com: From: Emil Velikov emil.velikov@collabora.com
With earlier commits we've removed DRM_AUTH for driver ioctls annotated with DRM_AUTH | DRM_RENDER_ALLOW, as the protection it introduces is effectively not existent.
With next commit, we'll effectively do the same for DRM core.
Yet the AMD developers have voiced concerns that by doing so, developers working on the closed source user-space driver might remove render node support.
Since we do _not_ want that to happen, add workaround for those two drivers
Cc: Alex Deucher alexander.deucher@amd.com Cc: Christian König christian.koenig@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com --- Christian, Alex this is the cleaner way to handle AMDGPU/radeon although if you prefer alternative methods let me know.
Review, acks and others are appreciated, since I'd like to get this through the drm-misc tree.
Thanks Emil
Unrelated: The USE_AGP flag in AMDGPU should be nuked. While for radeon, one can copy in the driver the 10-20 lines worth of agp_init/release and also drop the flag.
Bonus points of agp_init code gets a LEGACY check alongside the USE_AGP one. --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- drivers/gpu/drm/radeon/radeon_drv.c | 2 +- include/drm/drm_drv.h | 10 ++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 8e1b269351e8..cfc2ef11330c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1307,7 +1307,7 @@ amdgpu_get_crtc_scanout_position(struct drm_device *dev, unsigned int pipe,
static struct drm_driver kms_driver = { .driver_features = - DRIVER_USE_AGP | DRIVER_ATOMIC | + DRIVER_USE_AGP | DRIVER_ATOMIC | DRIVER_FORCE_AUTH | DRIVER_GEM | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ, .load = amdgpu_driver_load_kms, diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 4403e76e1ae0..5a1bfad1ad5e 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -538,7 +538,7 @@ radeon_get_crtc_scanout_position(struct drm_device *dev, unsigned int pipe,
static struct drm_driver kms_driver = { .driver_features = - DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER, + DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER | DRIVER_FORCE_AUTH, .load = radeon_driver_load_kms, .open = radeon_driver_open_kms, .postclose = radeon_driver_postclose_kms, diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index b33f2cee2099..5fb2846396bc 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -92,6 +92,16 @@ enum drm_driver_feature { * synchronization of command submission. */ DRIVER_SYNCOBJ_TIMELINE = BIT(6), + /** + * @DRIVER_FORCE_AUTH: + * + * Driver mandates that DRM_AUTH is honoured, even if the same ioctl + * is exposed via the render node - aka any of an "authentication" is + * a fallacy. + * + * Used only by amdgpu and radeon. Do not use. + */ + DRIVER_FORCE_AUTH = BIT(7),
/* IMPORTANT: Below are all the legacy flags, add new ones above. */
-- 2.21.0
On Wed, 3 Jul 2019 at 14:48, Koenig, Christian Christian.Koenig@amd.com wrote:
Well this is still a NAK.
As stated previously please just don't remove DRM_AUTH and keep the functionality as it is.
AFAICT nobody was in favour of your suggestion to remove DRM_AUTH from the handle to/from fd ioclts. Thus this seems like the second best option.
Third route that I see is doing driver_name == "amdgpu" || driver_name == "radeon" in core. If you have alternative solution I'm all ears.
-Emil
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)
v3: - Use single unlikely (Daniel V)
v4: - Reapply patch, use DRIVER_FORCE_AUTH w/a for amdgpu/radeon.
[1] https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859b... [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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_ioctl.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 09f7f8e33fa3..c30feb5e97e3 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -517,6 +517,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 * @@ -531,6 +538,8 @@ 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; @@ -538,7 +547,14 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) /* AUTH is only for authenticated or render client */ if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated)) - return -EACCES; + /* + * Although we allow: + * - render drivers with DRM_RENDER_ALLOW ioctls, when + * - drivers do not explicitly mandate authentication. + */ + if (!drm_render_driver_and_ioctl(dev, flags) || + drm_core_check_feature(dev, DRIVER_FORCE_AUTH)) + return -EACCES;
/* MASTER is only for master or control clients */ if (unlikely((flags & DRM_MASTER) &&
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)
v3: - Use single unlikely (Daniel V)
v4: - Reapply patch, check for amdgpu/radeon inline.
[1] https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859b... [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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- This version effectively supersedes the DRIVER_FORCE_AUTH flag introduced here.
https://lists.freedesktop.org/archives/dri-devel/2019-July/225165.html https://lists.freedesktop.org/archives/dri-devel/2019-July/225166.html --- drivers/gpu/drm/drm_ioctl.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 09f7f8e33fa3..ad7c67b89bdd 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -517,6 +517,29 @@ 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); +} + +/* + * Yet the AMD developers have voiced concerns that by allowing ioctls + * such as DRM_AUTH | DRM_RENDER_ALLOW w/o enforcing DRM_AUTH developers + * working on the closed source user-space driver might remove render + * node support. + * + * Since we do _not_ want that to happen, add workaround for those two + * drivers. + */ +static inline bool +is_amdgpu_or_radeon(const struct drm_device *dev) +{ + return ((dev->driver->name, "amdgpu") == 0 || + (dev->driver->name, "radeon") == 0); +} + /** * drm_ioctl_permit - Check ioctl permissions against caller * @@ -531,6 +554,8 @@ 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; @@ -538,7 +563,14 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) /* AUTH is only for authenticated or render client */ if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated)) - return -EACCES; + /* + * Although we allow: + * - render drivers with DRM_RENDER_ALLOW ioctls, when + * - we are not using amdgpu or radeon. + */ + if (!drm_render_driver_and_ioctl(dev, flags) || + is_amdgpu_or_radeon(dev)) + return -EACCES;
/* MASTER is only for master or control clients */ if (unlikely((flags & DRM_MASTER) &&
On 2019-07-03 7:10 p.m., 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)
v3:
- Use single unlikely (Daniel V)
v4:
- Reapply patch, check for amdgpu/radeon inline.
[1] https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859b... [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 Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
As discussed on IRC, IMHO this change requires more justification.
The system I'm writing this on has vainfo 2.4.0 installed, which opens a render node and works fine without this change.
Similarly, if kmscube hasn't been fixed to use a render node yet, surely it easily can.
You're asserting that the problem is wide-spread, and that fixing all broken userspace isn't feasible, but I haven't seen any evidence supporting that.
Since this change is essentially a workaround for broken userspace which can never have worked, and has the potential of subverting the ongoing transition from using primary nodes to render nodes in userspace code, there needs to be evidence supporting that the benefit outweighs the risk.
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Koenig, Christian -
Michel Dänzer