Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 27, 2019 at 10:47:39AM +0000, Koenig, Christian wrote:
> Am 27.05.19 um 10:17 schrieb Emil Velikov:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> > render node. A seemingly deliberate design decision.
> >
> > Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> > yet not all userspace checks if it's authenticated, but instead uses
> > uncommon assumptions.
> >
> > After days of digging through git log and testing, only a single (ab)use
> > was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> > assuming that failure implies lack of authentication.
> >
> > Affected versions are:
> >   - the whole 18.2.x series, which is EOL
> >   - the whole 18.3.x series, which is EOL
> >   - the 19.0.x series, prior to 19.0.4
> 
> Well you could have saved your time, cause this is still a NAK.
> 
> For the record: I strongly think that we don't want to expose any render 
> functionality on the primary node.
> 
> To even go a step further I would say that at least on AMD hardware we 
> should completely disable DRI2 for one of the next generations.
> 
> As a follow up I would then completely disallow the DRM authentication 
> for amdgpu, so that the command submission interface on the primary node 
> can only be used by the display server.

So amdgpu is running in one direction, while everyone else is running in
the other direction? Please note that your patch to change i915 landed
already, so that ship is sailing (but we could ofc revert that back
again).

Imo really not a great idea if we do a amdgpu vs. everyone else split
here. If we want to deprecate dri2/flink/rendering on primary nodes across
the stack, then that should be a stack wide decision, not an amdgpu one.

Same for the other one, i.e. this stuff here.
-Daniel

> 
> Regards,
> Christian.
> 
> >
> > Add a special quirk for that case, thus we can drop DRM_AUTH bits as
> > mentioned earlier.
> >
> > Since all the affected userspace is EOL, we also add a kconfig option
> > to disable this quirk.
> >
> > The whole approach is inspired by DRIVER_KMS_LEGACY_CONTEXT
> >
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: amd-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/amd/amdgpu/Kconfig      | 16 ++++++++++++++++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++++++++++-
> >   drivers/gpu/drm/drm_ioctl.c             |  5 +++++
> >   include/drm/drm_ioctl.h                 | 17 +++++++++++++++++
> >   4 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
> > index 9221e5489069..da415f445187 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> > +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> > @@ -40,6 +40,22 @@ config DRM_AMDGPU_GART_DEBUGFS
> >   	  Selecting this option creates a debugfs file to inspect the mapped
> >   	  pages. Uses more memory for housekeeping, enable only for debugging.
> >   
> > +config DRM_AMDGPU_FORCE_AUTH
> > +	bool "Force authentication check on AMDGPU_INFO ioctl"
> > +	default y
> > +	help
> > +	  There were some version of the Mesa RADV drivers, which relied on
> > +	  the ioctl failing, if the client is not authenticated.
> > +
> > +	  Namely, the following versions are affected:
> > +	    - the whole 18.2.x series, which is EOL
> > +	    - the whole 18.3.x series, which is EOL
> > +	    - the 19.0.x series, prior to 19.0.4
> > +
> > +	  Modern distributions, should disable this. That will allow various
> > +	  other clients to work, that would otherwise require root privileges.
> > +
> > +
> >   source "drivers/gpu/drm/amd/acp/Kconfig"
> >   source "drivers/gpu/drm/amd/display/Kconfig"
> >   source "drivers/gpu/drm/amd/amdkfd/Kconfig"
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index b17d0545728e..b8076929440b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -1214,7 +1214,17 @@ const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {
> >   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_MMAP, amdgpu_gem_mmap_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> >   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_WAIT_IDLE, amdgpu_gem_wait_idle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> >   	DRM_IOCTL_DEF_DRV(AMDGPU_CS, amdgpu_cs_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> > -	DRM_IOCTL_DEF_DRV(AMDGPU_INFO, amdgpu_info_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> > +	/* The DRM_FORCE_AUTH is effectively a workaround for the RADV Mesa driver.
> > +	 * This is required for Mesa:
> > +	 *  - the whole 18.2.x series, which is EOL
> > +	 *  - the whole 18.3.x series, which is EOL
> > +	 *  - the 19.0.x series, prior to 19.0.4
> > +	 */
> > +	DRM_IOCTL_DEF_DRV(AMDGPU_INFO, amdgpu_info_ioctl,
> > +#if defined(DRM_AMDGPU_FORCE_AUTH)
> > +		DRM_FORCE_AUTH|
> > +#endif
> > +		DRM_AUTH|DRM_RENDER_ALLOW),
> >   	DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_CS, amdgpu_cs_wait_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> >   	DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_FENCES, amdgpu_cs_wait_fences_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> >   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_METADATA, amdgpu_gem_metadata_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 2263e3ddd822..9841c0076f02 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -544,6 +544,11 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
> >   		     drm_is_render_client(file_priv)))
> >   		return -EACCES;
> >   
> > +	/* FORCE_AUTH is only for authenticated or render client */
> > +	if (unlikely((flags & DRM_FORCE_AUTH) && !drm_is_render_client(file_priv) &&
> > +		     !file_priv->authenticated))
> > +		return -EACCES;
> > +
> >   	return 0;
> >   }
> >   EXPORT_SYMBOL(drm_ioctl_permit);
> > diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h
> > index fafb6f592c4b..6084ee32043d 100644
> > --- a/include/drm/drm_ioctl.h
> > +++ b/include/drm/drm_ioctl.h
> > @@ -126,6 +126,23 @@ enum drm_ioctl_flags {
> >   	 * not set DRM_AUTH because they do not require authentication.
> >   	 */
> >   	DRM_RENDER_ALLOW	= BIT(5),
> > +	/**
> > +	 * @DRM_FORCE_AUTH:
> > +	 *
> > +	 * Authentication of the primary node is mandatory. Regardless that the
> > +	 * user can usually circumvent that by using the render node with exact
> > +	 * same ioctl.
> > +	 *
> > +	 * Note: this is effectively a workaround for AMDGPU AMDGPU_INFO ioctl
> > +	 * and the RADV Mesa driver. This is required for Mesa:
> > +	 *  - the whole 18.2.x series, which is EOL
> > +	 *  - the whole 18.3.x series, which is EOL
> > +	 *  - the 19.0.x series, prior to 19.0.4
> > +	 *
> > +	 * Note: later patch will effectively drop the DRM_AUTH for ioctls
> > +	 * annotated as DRM_AUTH | DRM_RENDER_ALLOW.
> > +	 */
> > +	DRM_FORCE_AUTH          = BIT(6),
> >   };
> >   
> >   /**
> 

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