[PATCH 1/3] drm/vmwgfx: check master authentication in surface_ref ioctls

[PATCH 1/3] drm/vmwgfx: check master authentication in surface_ref ioctls

[Emil Velikov]

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.
-- 
2.21.0

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

[PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Koenig, Christian]

[-- Attachment #1.1: Type: text/plain, Size: 3878 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 6503 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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/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-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) &&
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Emil Velikov]

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/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-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) &&
-- 
2.21.0

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

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

[Michel Dänzer]

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/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-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.


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Koenig, Christian]

[-- Attachment #1.1: Type: text/plain, Size: 943 bytes --]



Am 03.07.2019 16:00 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
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.

Well just keep it. As I said please don't change anything here.

Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the problems at hand far as I know.

And stopping those two at least prevents userspace to abuse this even more.

On the other hand I haven't seen any NAK on dropping DRM_AUTH from them.

Christian.



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


[-- Attachment #1.2: Type: text/html, Size: 2110 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

On Wed, 3 Jul 2019 at 15:33, Koenig, Christian <Christian.Koenig@amd.com> wrote:
> Am 03.07.2019 16:00 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
>
> 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.
>
>
> Well just keep it. As I said please don't change anything here.
>
> Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the problems at hand far as I know.
>
We also need the DRM_AUTH for handle to/from fd ones. Mesa drivers use
those ioctls.

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Koenig, Christian]

[-- Attachment #1.1: Type: text/plain, Size: 1358 bytes --]



Am 03.07.2019 16:51 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
On Wed, 3 Jul 2019 at 15:33, Koenig, Christian <Christian.Koenig@amd.com> wrote:
> Am 03.07.2019 16:00 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
>
> 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.
>
>
> Well just keep it. As I said please don't change anything here.
>
> Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the problems at hand far as I know.
>
We also need the DRM_AUTH for handle to/from fd ones. Mesa drivers use
those ioctls.

Yeah, but only for importing/exporting things.

And in those cases we either already gave render nodes or correctly authenticated primary nodes.

So no need to change anything here as far as I see.

I simply want to prevent that userspace gets the same functionality from the primary node they get from the render node. And that actually seems to be a good way to keep the restriction and still work around the userspace problems.

Christian.



-Emil


[-- Attachment #1.2: Type: text/html, Size: 2596 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

On Wed, 3 Jul 2019 at 15:58, Koenig, Christian <Christian.Koenig@amd.com> wrote:
> Am 03.07.2019 16:51 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
>
> On Wed, 3 Jul 2019 at 15:33, Koenig, Christian <Christian.Koenig@amd.com> wrote:
> > Am 03.07.2019 16:00 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
> >
> > 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.
> >
> >
> > Well just keep it. As I said please don't change anything here.
> >
> > Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the problems at hand far as I know.
> >
> We also need the DRM_AUTH for handle to/from fd ones. Mesa drivers use
> those ioctls.
>
>
> Yeah, but only for importing/exporting things.
>
> And in those cases we either already gave render nodes or correctly authenticated primary nodes.
>
> So no need to change anything here as far as I see.
>
Not quite. When working with the primary node we have the following scenarios:
 - handle to fd -> pass fd to other APIs - gbm, opencl, vdpau, etc
 - handle to fd -> fd to handle - use it internally

-Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Koenig, Christian]

Hi Emil,

Sorry for the delay, finally coming back to this after my vacation.

Am 03.07.19 um 17:14 schrieb Emil Velikov:
> On Wed, 3 Jul 2019 at 15:58, Koenig, Christian <Christian.Koenig@amd.com> wrote:
>> Am 03.07.2019 16:51 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
>>
>> On Wed, 3 Jul 2019 at 15:33, Koenig, Christian <Christian.Koenig@amd.com> wrote:
>>> Am 03.07.2019 16:00 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
>>>
>>> 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.
>>>
>>>
>>> Well just keep it. As I said please don't change anything here.
>>>
>>> Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the problems at hand far as I know.
>>>
>> We also need the DRM_AUTH for handle to/from fd ones. Mesa drivers use
>> those ioctls.
>>
>>
>> Yeah, but only for importing/exporting things.
>>
>> And in those cases we either already gave render nodes or correctly authenticated primary nodes.
>>
>> So no need to change anything here as far as I see.
>>
> Not quite. When working with the primary node we have the following scenarios:
>   - handle to fd -> pass fd to other APIs - gbm, opencl, vdpau, etc
>   - handle to fd -> fd to handle - use it internally

Yeah, but this would once more mean that we expose the same 
functionality on the primary node we do on the render node.

And that is exactly what I want to prevent, because I think it is a very 
bad idea and will result in basically deprecating the render node.

For the problem at hand it was sufficient to drop the DRM_AUTH flag from 
the driver IOCTLs and I don't see a reason why we should go further than 
this.

Please just completely drop this whole approach,
Christian.

>
> -Emil

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Koenig, Christian]

[-- Attachment #1.1: Type: text/plain, Size: 1775 bytes --]



Am 03.07.2019 17:14 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
On Wed, 3 Jul 2019 at 15:58, Koenig, Christian <Christian.Koenig@amd.com> wrote:
> Am 03.07.2019 16:51 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
>
> On Wed, 3 Jul 2019 at 15:33, Koenig, Christian <Christian.Koenig@amd.com> wrote:
> > Am 03.07.2019 16:00 schrieb Emil Velikov <emil.l.velikov@gmail.com>:
> >
> > 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.
> >
> >
> > Well just keep it. As I said please don't change anything here.
> >
> > Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the problems at hand far as I know.
> >
> We also need the DRM_AUTH for handle to/from fd ones. Mesa drivers use
> those ioctls.
>
>
> Yeah, but only for importing/exporting things.
>
> And in those cases we either already gave render nodes or correctly authenticated primary nodes.
>
> So no need to change anything here as far as I see.
>
Not quite. When working with the primary node we have the following scenarios:
 - handle to fd -> pass fd to other APIs - gbm, opencl, vdpau, etc
 - handle to fd -> fd to handle - use it internally

Yeah, I'm aware of that but hoped that this would be a rather rare case.

I need to take another closer look on this, but currently I am on vacation and won't have time next week either.

Trying to get back to this as soon as possible,
Christian.


-Emil


[-- Attachment #1.2: Type: text/html, Size: 3081 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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