[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/vmwgfx: add local DRM_AUTH check for PRIME TO/FROM HANDLE

[Emil Velikov]

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

Realistically no drivers, but vmwgfx care about the DRM_AUTH flag here.

Follow-up work in this driver will properly isolate primary clients from
different master realms, thus we'll no longer need to parse _any_ ioctl
flags.

Until that work lands, add a local workaround.

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
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 275d90fe2a25..32c18bb482a6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1131,6 +1131,15 @@ static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
 	} else if (!drm_ioctl_flags(nr, &flags))
 		return -EINVAL;
 
+	/*
+	 * Little workaround until the vmwgfx patches providing isolation of
+	 * primary clients from different master realms lands.
+	 * With that work, we'll no longer need to parse _any_ ioctl flags.
+	 */
+	if (nr == 0x2d /* DRM_IOCTL_PRIME_HANDLE_TO_FD */ ||
+	    nr == 0x2e /* DRM_IOCTL_PRIME_FD_TO_HANDLE */)
+		flags != DRM_AUTH;
+
 	vmaster = vmw_master_check(dev, file_priv, flags);
 	if (IS_ERR(vmaster)) {
 		ret = PTR_ERR(vmaster);
-- 
2.21.0

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

[PATCH 3/3] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

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

As mentioned by Christian, for drivers which support only primary nodes
this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.

For others, this check in particular will be a noop. So let's remove it
as suggested by Christian.

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>
---
 drivers/gpu/drm/drm_ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 09f7f8e33fa3..a397177af369 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -647,8 +647,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
 
-	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
-	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
-- 
2.21.0

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

Re: [PATCH 3/3] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Koenig, Christian]

Am 22.07.19 um 19:40 schrieb Emil Velikov:
> From: Emil Velikov <emil.velikov@collabora.com>
>
> As mentioned by Christian, for drivers which support only primary nodes
> this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
>
> For others, this check in particular will be a noop. So let's remove it
> as suggested by Christian.
>
> 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>

Looks like I can't convince you that opening up the primary node like 
this is a bad idea.

Well at least we don't need to add any new flags, so feel free to add my 
Acked-by.

Christian.

> ---
>   drivers/gpu/drm/drm_ioctl.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 09f7f8e33fa3..a397177af369 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -647,8 +647,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>   
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
>   
> -	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>   
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),

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

Re: [PATCH 2/3] drm/vmwgfx: add local DRM_AUTH check for PRIME TO/FROM HANDLE

[Emil Velikov]

On 2019/07/22, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Realistically no drivers, but vmwgfx care about the DRM_AUTH flag here.
> 
> Follow-up work in this driver will properly isolate primary clients from
> different master realms, thus we'll no longer need to parse _any_ ioctl
> flags.
> 
> Until that work lands, add a local workaround.
> 
> 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
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 275d90fe2a25..32c18bb482a6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1131,6 +1131,15 @@ static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
>  	} else if (!drm_ioctl_flags(nr, &flags))
>  		return -EINVAL;
>  
> +	/*
> +	 * Little workaround until the vmwgfx patches providing isolation of
> +	 * primary clients from different master realms lands.
> +	 * With that work, we'll no longer need to parse _any_ ioctl flags.
> +	 */
> +	if (nr == 0x2d /* DRM_IOCTL_PRIME_HANDLE_TO_FD */ ||
> +	    nr == 0x2e /* DRM_IOCTL_PRIME_FD_TO_HANDLE */)
> +		flags != DRM_AUTH;
> +
>  	vmaster = vmw_master_check(dev, file_priv, flags);
>  	if (IS_ERR(vmaster)) {
>  		ret = PTR_ERR(vmaster);
> -- 

Hi Deepak,

As far as I can tell Thomas is on holidays for another 2+ weeks.

Is there anyone else in the team who can review the VMWare patches of
this series? I tested the lot quickly, but additional confirmation would
be appreciated.

You can find the series via the "VMware Graphics" alias, or in the
patchwork link below.

https://patchwork.freedesktop.org/series/64024/

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

Re: [PATCH 2/3] drm/vmwgfx: add local DRM_AUTH check for PRIME TO/FROM HANDLE

[Deepak Singh Rawat]

> Hi Deepak,
> 
> As far as I can tell Thomas is on holidays for another 2+ weeks.
> 
> Is there anyone else in the team who can review the VMWare patches of
> this series? I tested the lot quickly, but additional confirmation
> would
> be appreciated.
> 
> You can find the series via the "VMware Graphics" alias, or in the
> patchwork link below.

Hi Emil,

I can look into your patches and I did had a cursory look at those and
to be honest I don't really know this area and also since it deals with
security I thought a RB from Thomas would be nice. I will devote some
more time on your patches. Thanks for doing this.

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

Re: [Linux-graphics-maintainer] [PATCH 2/3] drm/vmwgfx: add local DRM_AUTH check for PRIME TO/FROM HANDLE

[Deepak Singh Rawat]

On Mon, 2019-07-22 at 18:40 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Realistically no drivers, but vmwgfx care about the DRM_AUTH flag
> here.
> 
> Follow-up work in this driver will properly isolate primary clients
> from
> different master realms, thus we'll no longer need to parse _any_
> ioctl
> flags.
> 
> Until that work lands, add a local workaround.
> 
> 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
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 275d90fe2a25..32c18bb482a6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1131,6 +1131,15 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
>  	} else if (!drm_ioctl_flags(nr, &flags))
>  		return -EINVAL;
>  
> +	/*
> +	 * Little workaround until the vmwgfx patches providing
> isolation of
> +	 * primary clients from different master realms lands.
> +	 * With that work, we'll no longer need to parse _any_ ioctl
> flags.
> +	 */
> +	if (nr == 0x2d /* DRM_IOCTL_PRIME_HANDLE_TO_FD */ ||
> +	    nr == 0x2e /* DRM_IOCTL_PRIME_FD_TO_HANDLE */)
> +		flags != DRM_AUTH;

Do you mean bitwise OR assignment? In current form this is no-op.

> +
>  	vmaster = vmw_master_check(dev, file_priv, flags);
>  	if (IS_ERR(vmaster)) {
>  		ret = PTR_ERR(vmaster);

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