[PATCH 05/13] drm/i915: drop DRM_AUTH from DRM_RENDER_ALLOW ioctls

[PATCH 05/13] drm/i915: drop DRM_AUTH from DRM_RENDER_ALLOW ioctls

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

The authentication can be circumvented, by design, by using the render
node.

From the driver POV there is no distinction between primary and render
nodes, thus we can drop the token.

Note: the outstanding DRM_AUTH instances are:
 - legacy DRI1 ioctls, which are already neutered
 - modern but deprecated ioctls

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1ad88e6d7c04..ea7a713654dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -3098,7 +3098,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_BATCHBUFFER, drm_noop, DRM_AUTH),
 	DRM_IOCTL_DEF_DRV(I915_IRQ_EMIT, drm_noop, DRM_AUTH),
 	DRM_IOCTL_DEF_DRV(I915_IRQ_WAIT, drm_noop, DRM_AUTH),
-	DRM_IOCTL_DEF_DRV(I915_GETPARAM, i915_getparam_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GETPARAM, i915_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_SETPARAM, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_ALLOC, drm_noop, DRM_AUTH),
 	DRM_IOCTL_DEF_DRV(I915_FREE, drm_noop, DRM_AUTH),
@@ -3111,13 +3111,13 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_HWS_ADDR, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_GEM_INIT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER, i915_gem_execbuffer_ioctl, DRM_AUTH),
-	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2_WR, i915_gem_execbuffer2_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2_WR, i915_gem_execbuffer2_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_PIN, i915_gem_reject_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_GEM_UNPIN, i915_gem_reject_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY),
-	DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_SET_CACHING, i915_gem_set_caching_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_GET_CACHING, i915_gem_get_caching_ioctl, DRM_RENDER_ALLOW),
-	DRM_IOCTL_DEF_DRV(I915_GEM_THROTTLE, i915_gem_throttle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_THROTTLE, i915_gem_throttle_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_ENTERVT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_GEM_LEAVEVT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CREATE, i915_gem_create_ioctl, DRM_RENDER_ALLOW),
@@ -3136,7 +3136,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs_ioctl, DRM_MASTER),
 	DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey_ioctl, DRM_MASTER),
 	DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, drm_noop, DRM_MASTER),
-	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE_EXT, i915_gem_context_create_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_RENDER_ALLOW),
-- 
2.21.0

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

[PATCH 13/13] 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:
- 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-gfx@lists.freedesktop.org
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@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;
+	}
 
 	/* 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 05/13] drm/i915: drop DRM_AUTH from DRM_RENDER_ALLOW ioctls

[Jani Nikula]

On Mon, 27 May 2019, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
>
> The authentication can be circumvented, by design, by using the render
> node.
>
> From the driver POV there is no distinction between primary and render
> nodes, thus we can drop the token.
>
> Note: the outstanding DRM_AUTH instances are:
>  - legacy DRI1 ioctls, which are already neutered
>  - modern but deprecated ioctls
>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Please see

commit b972fffa114b18a120a7bbde105d69a080d24970
Author: Christian König <ckoenig.leichtzumerken@gmail.com>
Date:   Wed Apr 17 13:25:24 2019 +0200

    drm/i915: remove DRM_AUTH from IOCTLs which also have DRM_RENDER_ALLOW

It's headed to drm-next in [1].


BR,
Jani.


[1] http://mid.mail-archive.com/87sgt3n45z.fsf@intel.com


> ---
>  drivers/gpu/drm/i915/i915_drv.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1ad88e6d7c04..ea7a713654dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -3098,7 +3098,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_BATCHBUFFER, drm_noop, DRM_AUTH),
>  	DRM_IOCTL_DEF_DRV(I915_IRQ_EMIT, drm_noop, DRM_AUTH),
>  	DRM_IOCTL_DEF_DRV(I915_IRQ_WAIT, drm_noop, DRM_AUTH),
> -	DRM_IOCTL_DEF_DRV(I915_GETPARAM, i915_getparam_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GETPARAM, i915_getparam_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_SETPARAM, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(I915_ALLOC, drm_noop, DRM_AUTH),
>  	DRM_IOCTL_DEF_DRV(I915_FREE, drm_noop, DRM_AUTH),
> @@ -3111,13 +3111,13 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_HWS_ADDR, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_INIT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER, i915_gem_execbuffer_ioctl, DRM_AUTH),
> -	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2_WR, i915_gem_execbuffer2_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2_WR, i915_gem_execbuffer2_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_PIN, i915_gem_reject_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_UNPIN, i915_gem_reject_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY),
> -	DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_SET_CACHING, i915_gem_set_caching_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_GET_CACHING, i915_gem_get_caching_ioctl, DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF_DRV(I915_GEM_THROTTLE, i915_gem_throttle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GEM_THROTTLE, i915_gem_throttle_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_ENTERVT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_LEAVEVT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CREATE, i915_gem_create_ioctl, DRM_RENDER_ALLOW),
> @@ -3136,7 +3136,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs_ioctl, DRM_MASTER),
>  	DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey_ioctl, DRM_MASTER),
>  	DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, drm_noop, DRM_MASTER),
> -	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE_EXT, i915_gem_context_create_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_RENDER_ALLOW),

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Christian König]

Am 27.05.19 um 10:17 schrieb 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:
> - Patch was reverted because it broke AMDGPU, apply again. The AMDGPU
> issue is fixed with earlier patch.

As far as I can see this only affects the following two IOCTLs after 
removing DRM_AUTH from the DRM_RENDER_ALLOW IOCTLs:
> DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
> drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
>         DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
> drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW)

So I think it would be simpler to just remove DRM_AUTH from those two 
instead of allowing it for everybody.

Regards,
Christian.

>
> [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
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@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;
> +	}
>   
>   	/* MASTER is only for master or control clients */
>   	if (unlikely((flags & DRM_MASTER) &&

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

Re: [PATCH 05/13] drm/i915: drop DRM_AUTH from DRM_RENDER_ALLOW ioctls

[Emil Velikov]

On 2019/05/27, Jani Nikula wrote:
> On Mon, 27 May 2019, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > The authentication can be circumvented, by design, by using the render
> > node.
> >
> > From the driver POV there is no distinction between primary and render
> > nodes, thus we can drop the token.
> >
> > Note: the outstanding DRM_AUTH instances are:
> >  - legacy DRI1 ioctls, which are already neutered
> >  - modern but deprecated ioctls
> >
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> 
> Please see
> 
> commit b972fffa114b18a120a7bbde105d69a080d24970
> Author: Christian König <ckoenig.leichtzumerken@gmail.com>
> Date:   Wed Apr 17 13:25:24 2019 +0200
> 
>     drm/i915: remove DRM_AUTH from IOCTLs which also have DRM_RENDER_ALLOW
> 
> It's headed to drm-next in [1].
> 
Right my series is based on drm-misc-next, which does not (yet) have
the patch. I'll drop my patch when the equivalent, shows up in
drm-misc-next.

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

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

[Emil Velikov]

On 2019/05/27, Christian König wrote:
> Am 27.05.19 um 10:17 schrieb 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:
> > - Patch was reverted because it broke AMDGPU, apply again. The AMDGPU
> > issue is fixed with earlier patch.
> 
> As far as I can see this only affects the following two IOCTLs after
> removing DRM_AUTH from the DRM_RENDER_ALLOW IOCTLs:
> > DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD,
> > drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
> >         DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE,
> > drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW)
> 
> So I think it would be simpler to just remove DRM_AUTH from those two
> instead of allowing it for everybody.
> 
If I understand you correctly this will remove DRM_AUTH also for drivers
which expose only a primary node. I'm not sure if that is a good idea.

That said, if others are OK with the idea I will prepare a patch.

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

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

[Koenig, Christian]

Am 27.05.19 um 14:10 schrieb Emil Velikov:
> On 2019/05/27, Christian König wrote:
>> Am 27.05.19 um 10:17 schrieb 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:
>>> - Patch was reverted because it broke AMDGPU, apply again. The AMDGPU
>>> issue is fixed with earlier patch.
>> As far as I can see this only affects the following two IOCTLs after
>> removing DRM_AUTH from the DRM_RENDER_ALLOW IOCTLs:
>>> DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD,
>>> drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>          DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE,
>>> drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW)
>> So I think it would be simpler to just remove DRM_AUTH from those two
>> instead of allowing it for everybody.
>>
> If I understand you correctly this will remove DRM_AUTH also for drivers
> which expose only a primary node. I'm not sure if that is a good idea.

That's a good point, but I have doubts that those drivers implement the 
necessary callbacks and/or set the core feature flag for the IOCTLs.

So the maximum what could happen is that you change the returned error 
from -EACCES into -EOPNOTSUPP/-ENOSYS.

Regards,
Christian.

> That said, if others are OK with the idea I will prepare a patch.
>
> Thanks
> Emil

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

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

[Thomas Hellstrom]

On 5/27/19 10:17 AM, 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:
> - 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-gfx@lists.freedesktop.org
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@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.

/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

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

[Emil Velikov]

On 2019/05/27, Thomas Hellstrom wrote:
> On 5/27/19 10:17 AM, 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:
> > - 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-gfx@lists.freedesktop.org
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@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.
> 
Ack. Thanks for having a look. Will include a permission check in v2
of the series.

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

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

[Daniel Vetter]

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.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:
> > - 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-gfx@lists.freedesktop.org
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@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

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Thomas Hellstrom]

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.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:
>>> - 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-gfx@lists.freedesktop.org
>>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@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


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

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

[Daniel Vetter]

On Mon, May 27, 2019 at 4:01 PM Thomas Hellstrom <thomas@shipmail.org> wrote:
>
> 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.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:
> >>> - 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-gfx@lists.freedesktop.org
> >>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@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....

Uh, I read something completely different in there. I guess I didn't
really follow what's going on there :-)
-Daniel

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel