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

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

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

[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