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

["Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>]

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.

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),
>   };
>   
>   /**

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