[PATCH 1/5] vmwgfx: drop empty lastclose stub

[PATCH 1/5] vmwgfx: drop empty lastclose stub

[Emil Velikov]

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

Core DRM is safe when the callback is NULL.

Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index be25ce9440ad..a38f06909fb6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1200,10 +1200,6 @@ static long vmw_compat_ioctl(struct file *filp, unsigned int cmd,
 }
 #endif
 
-static void vmw_lastclose(struct drm_device *dev)
-{
-}
-
 static void vmw_master_init(struct vmw_master *vmaster)
 {
 	ttm_lock_init(&vmaster->lock);
@@ -1568,7 +1564,6 @@ static struct drm_driver driver = {
 	DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC,
 	.load = vmw_driver_load,
 	.unload = vmw_driver_unload,
-	.lastclose = vmw_lastclose,
 	.get_vblank_counter = vmw_get_vblank_counter,
 	.enable_vblank = vmw_enable_vblank,
 	.disable_vblank = vmw_disable_vblank,
-- 
2.21.0

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

[PATCH 2/5] drm/vmgfx: kill off unused init_mutex

[Emil Velikov]

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

According to the docs - prevents firstopen/lastclose races. Yet never
used in practise.

Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index a38f06909fb6..d3f108f7e52d 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -664,7 +664,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 		INIT_LIST_HEAD(&dev_priv->res_lru[i]);
 	}
 
-	mutex_init(&dev_priv->init_mutex);
 	init_waitqueue_head(&dev_priv->fence_queue);
 	init_waitqueue_head(&dev_priv->fifo_queue);
 	dev_priv->fence_queue_waiters = 0;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 96983c47fb40..9be2176cc260 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -484,11 +484,6 @@ struct vmw_private {
 
 	spinlock_t resource_lock;
 	struct idr res_idr[vmw_res_max];
-	/*
-	 * Block lastclose from racing with firstopen.
-	 */
-
-	struct mutex init_mutex;
 
 	/*
 	 * A resource manager for kernel-only surfaces and
-- 
2.21.0

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

[PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Emil Velikov]

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

Currently vmw_execbuf_ioctl() open-codes the permission checking, size
extending and copying that is already done in core drm.

Kill all the duplication, adding a few comments for clarity.

Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Thomas, VMware team,

Please give this some testing on your end. I've only tested it against
mesa-master.

Thanks
Emil
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 12 +-----
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  4 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++----------------
 3 files changed, 23 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index d3f108f7e52d..2cb6ae219e43 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -186,7 +186,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
 		      DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
 		      DRM_AUTH | DRM_RENDER_ALLOW),
-	VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
+	VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
 		      DRM_RENDER_ALLOW),
 	VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
 		      DRM_RENDER_ALLOW),
@@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
 			&vmw_ioctls[nr - DRM_COMMAND_BASE];
 
 		if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
-			ret = (long) drm_ioctl_permit(ioctl->flags, file_priv);
-			if (unlikely(ret != 0))
-				return ret;
-
-			if (unlikely((cmd & (IOC_IN | IOC_OUT)) != IOC_IN))
-				goto out_io_encoding;
-
-			return (long) vmw_execbuf_ioctl(dev, arg, file_priv,
-							_IOC_SIZE(cmd));
+			return ioctl_func(filp, cmd, arg);
 		} else if (nr == DRM_COMMAND_BASE + DRM_VMW_UPDATE_LAYOUT) {
 			if (!drm_is_current_master(file_priv) &&
 			    !capable(CAP_SYS_ADMIN))
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 9be2176cc260..f5bfac85f793 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -910,8 +910,8 @@ static inline struct page *vmw_piter_page(struct vmw_piter *viter)
  * Command submission - vmwgfx_execbuf.c
  */
 
-extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
-			     struct drm_file *file_priv, size_t size);
+extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
+			     struct drm_file *file_priv);
 extern int vmw_execbuf_process(struct drm_file *file_priv,
 			       struct vmw_private *dev_priv,
 			       void __user *user_commands,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 2ff7ba04d8c8..767e2b99618d 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct vmw_private *dev_priv)
 	mutex_unlock(&dev_priv->cmdbuf_mutex);
 }
 
-int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
-		      struct drm_file *file_priv, size_t size)
+int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
+		      struct drm_file *file_priv)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
-	struct drm_vmw_execbuf_arg arg;
+	struct drm_vmw_execbuf_arg *arg = data;
 	int ret;
-	static const size_t copy_offset[] = {
-		offsetof(struct drm_vmw_execbuf_arg, context_handle),
-		sizeof(struct drm_vmw_execbuf_arg)};
 	struct dma_fence *in_fence = NULL;
 
-	if (unlikely(size < copy_offset[0])) {
-		VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
-			       DRM_VMW_EXECBUF);
-		return -EINVAL;
-	}
-
-	if (copy_from_user(&arg, (void __user *) data, copy_offset[0]) != 0)
-		return -EFAULT;
-
 	/*
 	 * Extend the ioctl argument while maintaining backwards compatibility:
-	 * We take different code paths depending on the value of arg.version.
+	 * We take different code paths depending on the value of arg->version.
+	 *
+	 * Note: The ioctl argument is extended and zeropadded by core DRM.
 	 */
-	if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
-		     arg.version == 0)) {
+	if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
+		     arg->version == 0)) {
 		VMW_DEBUG_USER("Incorrect execbuf version.\n");
 		return -EINVAL;
 	}
 
-	if (arg.version > 1 &&
-	    copy_from_user(&arg.context_handle,
-			   (void __user *) (data + copy_offset[0]),
-			   copy_offset[arg.version - 1] - copy_offset[0]) != 0)
-		return -EFAULT;
-
-	switch (arg.version) {
+	switch (arg->version) {
 	case 1:
-		arg.context_handle = (uint32_t) -1;
+		/* For v1 core DRM have extended + zeropadded the data */
+		arg->context_handle = (uint32_t) -1;
 		break;
 	case 2:
 	default:
+		/* For v2 and later core DRM would have correctly copied it */
 		break;
 	}
 
 	/* If imported a fence FD from elsewhere, then wait on it */
-	if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
-		in_fence = sync_file_get_fence(arg.imported_fence_fd);
+	if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
+		in_fence = sync_file_get_fence(arg->imported_fence_fd);
 
 		if (!in_fence) {
 			VMW_DEBUG_USER("Cannot get imported fence\n");
@@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
 		return ret;
 
 	ret = vmw_execbuf_process(file_priv, dev_priv,
-				  (void __user *)(unsigned long)arg.commands,
-				  NULL, arg.command_size, arg.throttle_us,
-				  arg.context_handle,
-				  (void __user *)(unsigned long)arg.fence_rep,
-				  NULL, arg.flags);
+				  (void __user *)(unsigned long)arg->commands,
+				  NULL, arg->command_size, arg->throttle_us,
+				  arg->context_handle,
+				  (void __user *)(unsigned long)arg->fence_rep,
+				  NULL, arg->flags);
 
 	ttm_read_unlock(&dev_priv->reservation_sem);
 	if (unlikely(ret != 0))
-- 
2.21.0

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

[PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

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

Drop the custom ioctl io encoding check - core drm does it for us.

Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 2cb6ae219e43..f65542639b55 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1147,9 +1147,6 @@ static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
 				return -EACCES;
 		}
 
-		if (unlikely(ioctl->cmd != cmd))
-			goto out_io_encoding;
-
 		flags = ioctl->flags;
 	} else if (!drm_ioctl_flags(nr, &flags))
 		return -EINVAL;
@@ -1169,12 +1166,6 @@ static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
 		ttm_read_unlock(&vmaster->lock);
 
 	return ret;
-
-out_io_encoding:
-	DRM_ERROR("Invalid command format, ioctl %d\n",
-		  nr - DRM_COMMAND_BASE);
-
-	return -EINVAL;
 }
 
 static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd,
-- 
2.21.0

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

[PATCH 5/5] drm: make drm_ioctl_permit() internal

[Emil Velikov]

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

As of earlier commit the function is used only within drm core.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/drm_ioctl.c | 3 +--
 include/drm/drm_ioctl.h     | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 2263e3ddd822..0646c0bd5535 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -523,7 +523,7 @@ int drm_version(struct drm_device *dev, void *data,
  * Returns:
  * Zero if allowed, -EACCES otherwise.
  */
-int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
+static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 {
 	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
 	if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)))
@@ -546,7 +546,6 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 
 	return 0;
 }
-EXPORT_SYMBOL(drm_ioctl_permit);
 
 #define DRM_IOCTL_DEF(ioctl, _func, _flags)	\
 	[DRM_IOCTL_NR(ioctl)] = {		\
diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h
index fafb6f592c4b..f3fba529cb1b 100644
--- a/include/drm/drm_ioctl.h
+++ b/include/drm/drm_ioctl.h
@@ -163,7 +163,6 @@ struct drm_ioctl_desc {
 		.name = #ioctl						\
 	}
 
-int drm_ioctl_permit(u32 flags, struct drm_file *file_priv);
 long drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 long drm_ioctl_kernel(struct file *, drm_ioctl_t, void *, u32);
 #ifdef CONFIG_COMPAT
-- 
2.21.0

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

Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Thomas Hellstrom]

Hi, Emil,

On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently vmw_execbuf_ioctl() open-codes the permission checking,
> size
> extending and copying that is already done in core drm.
> 
> Kill all the duplication, adding a few comments for clarity.

Ah, there is core functionality for this now.

What worries me though with the core approach is that the sizes are not
capped by the size of the kernel argument definition, which makes
mailicious user-space being able to force kmallocs() the size of the
maximum ioctl size. Should probably be fixed before pushing this.


> 
> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Thomas, VMware team,
> 
> Please give this some testing on your end. I've only tested it
> against
> mesa-master.

I'll review tomorrow and do some testing. Need to see if I can dig up
user-space apps with version 0...

Thanks,

Thomas

> 
> Thanks
> Emil
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 12 +-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++------------
> ----
>  3 files changed, 23 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index d3f108f7e52d..2cb6ae219e43 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -186,7 +186,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] =
> {
>  		      DRM_RENDER_ALLOW),
>  	VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
>  		      DRM_AUTH | DRM_RENDER_ALLOW),
> -	VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
> +	VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
>  		      DRM_RENDER_ALLOW),
>  	VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
>  		      DRM_RENDER_ALLOW),
> @@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
>  			&vmw_ioctls[nr - DRM_COMMAND_BASE];
>  
>  		if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
> -			ret = (long) drm_ioctl_permit(ioctl->flags,
> file_priv);
> -			if (unlikely(ret != 0))
> -				return ret;
> -
> -			if (unlikely((cmd & (IOC_IN | IOC_OUT)) !=
> IOC_IN))
> -				goto out_io_encoding;
> -
> -			return (long) vmw_execbuf_ioctl(dev, arg,
> file_priv,
> -							_IOC_SIZE(cmd))
> ;
> +			return ioctl_func(filp, cmd, arg);
>  		} else if (nr == DRM_COMMAND_BASE +
> DRM_VMW_UPDATE_LAYOUT) {
>  			if (!drm_is_current_master(file_priv) &&
>  			    !capable(CAP_SYS_ADMIN))
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 9be2176cc260..f5bfac85f793 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -910,8 +910,8 @@ static inline struct page *vmw_piter_page(struct
> vmw_piter *viter)
>   * Command submission - vmwgfx_execbuf.c
>   */
>  
> -extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long
> data,
> -			     struct drm_file *file_priv, size_t size);
> +extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> +			     struct drm_file *file_priv);
>  extern int vmw_execbuf_process(struct drm_file *file_priv,
>  			       struct vmw_private *dev_priv,
>  			       void __user *user_commands,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 2ff7ba04d8c8..767e2b99618d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct
> vmw_private *dev_priv)
>  	mutex_unlock(&dev_priv->cmdbuf_mutex);
>  }
>  
> -int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
> -		      struct drm_file *file_priv, size_t size)
> +int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> +		      struct drm_file *file_priv)
>  {
>  	struct vmw_private *dev_priv = vmw_priv(dev);
> -	struct drm_vmw_execbuf_arg arg;
> +	struct drm_vmw_execbuf_arg *arg = data;
>  	int ret;
> -	static const size_t copy_offset[] = {
> -		offsetof(struct drm_vmw_execbuf_arg, context_handle),
> -		sizeof(struct drm_vmw_execbuf_arg)};
>  	struct dma_fence *in_fence = NULL;
>  
> -	if (unlikely(size < copy_offset[0])) {
> -		VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
> -			       DRM_VMW_EXECBUF);
> -		return -EINVAL;
> -	}
> -
> -	if (copy_from_user(&arg, (void __user *) data, copy_offset[0])
> != 0)
> -		return -EFAULT;
> -
>  	/*
>  	 * Extend the ioctl argument while maintaining backwards
> compatibility:
> -	 * We take different code paths depending on the value of
> arg.version.
> +	 * We take different code paths depending on the value of arg-
> >version.
> +	 *
> +	 * Note: The ioctl argument is extended and zeropadded by core
> DRM.
>  	 */
> -	if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
> -		     arg.version == 0)) {
> +	if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
> +		     arg->version == 0)) {
>  		VMW_DEBUG_USER("Incorrect execbuf version.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (arg.version > 1 &&
> -	    copy_from_user(&arg.context_handle,
> -			   (void __user *) (data + copy_offset[0]),
> -			   copy_offset[arg.version - 1] -
> copy_offset[0]) != 0)
> -		return -EFAULT;
> -
> -	switch (arg.version) {
> +	switch (arg->version) {
>  	case 1:
> -		arg.context_handle = (uint32_t) -1;
> +		/* For v1 core DRM have extended + zeropadded the data
> */
> +		arg->context_handle = (uint32_t) -1;
>  		break;
>  	case 2:
>  	default:
> +		/* For v2 and later core DRM would have correctly
> copied it */
>  		break;
>  	}
>  
>  	/* If imported a fence FD from elsewhere, then wait on it */
> -	if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> -		in_fence = sync_file_get_fence(arg.imported_fence_fd);
> +	if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> +		in_fence = sync_file_get_fence(arg->imported_fence_fd);
>  
>  		if (!in_fence) {
>  			VMW_DEBUG_USER("Cannot get imported fence\n");
> @@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device *dev,
> unsigned long data,
>  		return ret;
>  
>  	ret = vmw_execbuf_process(file_priv, dev_priv,
> -				  (void __user *)(unsigned
> long)arg.commands,
> -				  NULL, arg.command_size,
> arg.throttle_us,
> -				  arg.context_handle,
> -				  (void __user *)(unsigned
> long)arg.fence_rep,
> -				  NULL, arg.flags);
> +				  (void __user *)(unsigned long)arg-
> >commands,
> +				  NULL, arg->command_size, arg-
> >throttle_us,
> +				  arg->context_handle,
> +				  (void __user *)(unsigned long)arg-
> >fence_rep,
> +				  NULL, arg->flags);
>  
>  	ttm_read_unlock(&dev_priv->reservation_sem);
>  	if (unlikely(ret != 0))
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Daniel Vetter]

On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <thellstrom@vmware.com> wrote:
>
> Hi, Emil,
>
> On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > Currently vmw_execbuf_ioctl() open-codes the permission checking,
> > size
> > extending and copying that is already done in core drm.
> >
> > Kill all the duplication, adding a few comments for clarity.
>
> Ah, there is core functionality for this now.
>
> What worries me though with the core approach is that the sizes are not
> capped by the size of the kernel argument definition, which makes
> mailicious user-space being able to force kmallocs() the size of the
> maximum ioctl size. Should probably be fixed before pushing this.

Hm I always worked under the assumption that kmalloc and friends
should be userspace hardened. Otherwise stuff like kmalloc_array
doesn't make any sense, everyone just feeds it unchecked input and
expects that helper to handle overflows.

If we assume kmalloc isn't hardened against that, then we have a much
bigger problem than just vmwgfx ioctls ...
-Daniel

>
>
> >
> > Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > Thomas, VMware team,
> >
> > Please give this some testing on your end. I've only tested it
> > against
> > mesa-master.
>
> I'll review tomorrow and do some testing. Need to see if I can dig up
> user-space apps with version 0...
>
> Thanks,
>
> Thomas
>
> >
> > Thanks
> > Emil
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 12 +-----
> >  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  4 +-
> >  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++------------
> > ----
> >  3 files changed, 23 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > index d3f108f7e52d..2cb6ae219e43 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > @@ -186,7 +186,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] =
> > {
> >                     DRM_RENDER_ALLOW),
> >       VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
> >                     DRM_AUTH | DRM_RENDER_ALLOW),
> > -     VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
> > +     VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
> >                     DRM_RENDER_ALLOW),
> >       VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
> >                     DRM_RENDER_ALLOW),
> > @@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file
> > *filp, unsigned int cmd,
> >                       &vmw_ioctls[nr - DRM_COMMAND_BASE];
> >
> >               if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
> > -                     ret = (long) drm_ioctl_permit(ioctl->flags,
> > file_priv);
> > -                     if (unlikely(ret != 0))
> > -                             return ret;
> > -
> > -                     if (unlikely((cmd & (IOC_IN | IOC_OUT)) !=
> > IOC_IN))
> > -                             goto out_io_encoding;
> > -
> > -                     return (long) vmw_execbuf_ioctl(dev, arg,
> > file_priv,
> > -                                                     _IOC_SIZE(cmd))
> > ;
> > +                     return ioctl_func(filp, cmd, arg);
> >               } else if (nr == DRM_COMMAND_BASE +
> > DRM_VMW_UPDATE_LAYOUT) {
> >                       if (!drm_is_current_master(file_priv) &&
> >                           !capable(CAP_SYS_ADMIN))
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > index 9be2176cc260..f5bfac85f793 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > @@ -910,8 +910,8 @@ static inline struct page *vmw_piter_page(struct
> > vmw_piter *viter)
> >   * Command submission - vmwgfx_execbuf.c
> >   */
> >
> > -extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long
> > data,
> > -                          struct drm_file *file_priv, size_t size);
> > +extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> > +                          struct drm_file *file_priv);
> >  extern int vmw_execbuf_process(struct drm_file *file_priv,
> >                              struct vmw_private *dev_priv,
> >                              void __user *user_commands,
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > index 2ff7ba04d8c8..767e2b99618d 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > @@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct
> > vmw_private *dev_priv)
> >       mutex_unlock(&dev_priv->cmdbuf_mutex);
> >  }
> >
> > -int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
> > -                   struct drm_file *file_priv, size_t size)
> > +int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> > +                   struct drm_file *file_priv)
> >  {
> >       struct vmw_private *dev_priv = vmw_priv(dev);
> > -     struct drm_vmw_execbuf_arg arg;
> > +     struct drm_vmw_execbuf_arg *arg = data;
> >       int ret;
> > -     static const size_t copy_offset[] = {
> > -             offsetof(struct drm_vmw_execbuf_arg, context_handle),
> > -             sizeof(struct drm_vmw_execbuf_arg)};
> >       struct dma_fence *in_fence = NULL;
> >
> > -     if (unlikely(size < copy_offset[0])) {
> > -             VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
> > -                            DRM_VMW_EXECBUF);
> > -             return -EINVAL;
> > -     }
> > -
> > -     if (copy_from_user(&arg, (void __user *) data, copy_offset[0])
> > != 0)
> > -             return -EFAULT;
> > -
> >       /*
> >        * Extend the ioctl argument while maintaining backwards
> > compatibility:
> > -      * We take different code paths depending on the value of
> > arg.version.
> > +      * We take different code paths depending on the value of arg-
> > >version.
> > +      *
> > +      * Note: The ioctl argument is extended and zeropadded by core
> > DRM.
> >        */
> > -     if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
> > -                  arg.version == 0)) {
> > +     if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
> > +                  arg->version == 0)) {
> >               VMW_DEBUG_USER("Incorrect execbuf version.\n");
> >               return -EINVAL;
> >       }
> >
> > -     if (arg.version > 1 &&
> > -         copy_from_user(&arg.context_handle,
> > -                        (void __user *) (data + copy_offset[0]),
> > -                        copy_offset[arg.version - 1] -
> > copy_offset[0]) != 0)
> > -             return -EFAULT;
> > -
> > -     switch (arg.version) {
> > +     switch (arg->version) {
> >       case 1:
> > -             arg.context_handle = (uint32_t) -1;
> > +             /* For v1 core DRM have extended + zeropadded the data
> > */
> > +             arg->context_handle = (uint32_t) -1;
> >               break;
> >       case 2:
> >       default:
> > +             /* For v2 and later core DRM would have correctly
> > copied it */
> >               break;
> >       }
> >
> >       /* If imported a fence FD from elsewhere, then wait on it */
> > -     if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> > -             in_fence = sync_file_get_fence(arg.imported_fence_fd);
> > +     if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> > +             in_fence = sync_file_get_fence(arg->imported_fence_fd);
> >
> >               if (!in_fence) {
> >                       VMW_DEBUG_USER("Cannot get imported fence\n");
> > @@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device *dev,
> > unsigned long data,
> >               return ret;
> >
> >       ret = vmw_execbuf_process(file_priv, dev_priv,
> > -                               (void __user *)(unsigned
> > long)arg.commands,
> > -                               NULL, arg.command_size,
> > arg.throttle_us,
> > -                               arg.context_handle,
> > -                               (void __user *)(unsigned
> > long)arg.fence_rep,
> > -                               NULL, arg.flags);
> > +                               (void __user *)(unsigned long)arg-
> > >commands,
> > +                               NULL, arg->command_size, arg-
> > >throttle_us,
> > +                               arg->context_handle,
> > +                               (void __user *)(unsigned long)arg-
> > >fence_rep,
> > +                               NULL, arg->flags);
> >
> >       ttm_read_unlock(&dev_priv->reservation_sem);
> >       if (unlikely(ret != 0))



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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom]

Hi, Emil,

On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Drop the custom ioctl io encoding check - core drm does it for us.

I fail to see where the core does this, or do I miss something?
Thanks,
Thomas


> 
> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 2cb6ae219e43..f65542639b55 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1147,9 +1147,6 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
>  				return -EACCES;
>  		}
>  
> -		if (unlikely(ioctl->cmd != cmd))
> -			goto out_io_encoding;
> -
>  		flags = ioctl->flags;
>  	} else if (!drm_ioctl_flags(nr, &flags))
>  		return -EINVAL;
> @@ -1169,12 +1166,6 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
>  		ttm_read_unlock(&vmaster->lock);
>  
>  	return ret;
> -
> -out_io_encoding:
> -	DRM_ERROR("Invalid command format, ioctl %d\n",
> -		  nr - DRM_COMMAND_BASE);
> -
> -	return -EINVAL;
>  }
>  
>  static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] vmwgfx: drop empty lastclose stub

[Thomas Hellstrom]

On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Core DRM is safe when the callback is NULL.
> 
> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>


> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index be25ce9440ad..a38f06909fb6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1200,10 +1200,6 @@ static long vmw_compat_ioctl(struct file
> *filp, unsigned int cmd,
>  }
>  #endif
>  
> -static void vmw_lastclose(struct drm_device *dev)
> -{
> -}
> -
>  static void vmw_master_init(struct vmw_master *vmaster)
>  {
>  	ttm_lock_init(&vmaster->lock);
> @@ -1568,7 +1564,6 @@ static struct drm_driver driver = {
>  	DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC,
>  	.load = vmw_driver_load,
>  	.unload = vmw_driver_unload,
> -	.lastclose = vmw_lastclose,
>  	.get_vblank_counter = vmw_get_vblank_counter,
>  	.enable_vblank = vmw_enable_vblank,
>  	.disable_vblank = vmw_disable_vblank,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/vmgfx: kill off unused init_mutex

[Thomas Hellstrom]

On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> According to the docs - prevents firstopen/lastclose races. Yet never
> used in practise.
> 
> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>

> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 5 -----
>  2 files changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index a38f06909fb6..d3f108f7e52d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -664,7 +664,6 @@ static int vmw_driver_load(struct drm_device
> *dev, unsigned long chipset)
>  		INIT_LIST_HEAD(&dev_priv->res_lru[i]);
>  	}
>  
> -	mutex_init(&dev_priv->init_mutex);
>  	init_waitqueue_head(&dev_priv->fence_queue);
>  	init_waitqueue_head(&dev_priv->fifo_queue);
>  	dev_priv->fence_queue_waiters = 0;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 96983c47fb40..9be2176cc260 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -484,11 +484,6 @@ struct vmw_private {
>  
>  	spinlock_t resource_lock;
>  	struct idr res_idr[vmw_res_max];
> -	/*
> -	 * Block lastclose from racing with firstopen.
> -	 */
> -
> -	struct mutex init_mutex;
>  
>  	/*
>  	 * A resource manager for kernel-only surfaces and
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Thomas Hellstrom]

On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently vmw_execbuf_ioctl() open-codes the permission checking,
> size
> extending and copying that is already done in core drm.
> 
> Kill all the duplication, adding a few comments for clarity.
> 
> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Tested using piglit quick using execbuf versions 1 and 2.

Tested-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>


> ---
> Thomas, VMware team,
> 
> Please give this some testing on your end. I've only tested it
> against
> mesa-master.
> 
> Thanks
> Emil
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 12 +-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++------------
> ----
>  3 files changed, 23 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index d3f108f7e52d..2cb6ae219e43 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -186,7 +186,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] =
> {
>  		      DRM_RENDER_ALLOW),
>  	VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
>  		      DRM_AUTH | DRM_RENDER_ALLOW),
> -	VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
> +	VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
>  		      DRM_RENDER_ALLOW),
>  	VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
>  		      DRM_RENDER_ALLOW),
> @@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
>  			&vmw_ioctls[nr - DRM_COMMAND_BASE];
>  
>  		if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
> -			ret = (long) drm_ioctl_permit(ioctl->flags,
> file_priv);
> -			if (unlikely(ret != 0))
> -				return ret;
> -
> -			if (unlikely((cmd & (IOC_IN | IOC_OUT)) !=
> IOC_IN))
> -				goto out_io_encoding;
> -
> -			return (long) vmw_execbuf_ioctl(dev, arg,
> file_priv,
> -							_IOC_SIZE(cmd))
> ;
> +			return ioctl_func(filp, cmd, arg);
>  		} else if (nr == DRM_COMMAND_BASE +
> DRM_VMW_UPDATE_LAYOUT) {
>  			if (!drm_is_current_master(file_priv) &&
>  			    !capable(CAP_SYS_ADMIN))
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 9be2176cc260..f5bfac85f793 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -910,8 +910,8 @@ static inline struct page *vmw_piter_page(struct
> vmw_piter *viter)
>   * Command submission - vmwgfx_execbuf.c
>   */
>  
> -extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long
> data,
> -			     struct drm_file *file_priv, size_t size);
> +extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> +			     struct drm_file *file_priv);
>  extern int vmw_execbuf_process(struct drm_file *file_priv,
>  			       struct vmw_private *dev_priv,
>  			       void __user *user_commands,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 2ff7ba04d8c8..767e2b99618d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct
> vmw_private *dev_priv)
>  	mutex_unlock(&dev_priv->cmdbuf_mutex);
>  }
>  
> -int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
> -		      struct drm_file *file_priv, size_t size)
> +int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> +		      struct drm_file *file_priv)
>  {
>  	struct vmw_private *dev_priv = vmw_priv(dev);
> -	struct drm_vmw_execbuf_arg arg;
> +	struct drm_vmw_execbuf_arg *arg = data;
>  	int ret;
> -	static const size_t copy_offset[] = {
> -		offsetof(struct drm_vmw_execbuf_arg, context_handle),
> -		sizeof(struct drm_vmw_execbuf_arg)};
>  	struct dma_fence *in_fence = NULL;
>  
> -	if (unlikely(size < copy_offset[0])) {
> -		VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
> -			       DRM_VMW_EXECBUF);
> -		return -EINVAL;
> -	}
> -
> -	if (copy_from_user(&arg, (void __user *) data, copy_offset[0])
> != 0)
> -		return -EFAULT;
> -
>  	/*
>  	 * Extend the ioctl argument while maintaining backwards
> compatibility:
> -	 * We take different code paths depending on the value of
> arg.version.
> +	 * We take different code paths depending on the value of arg-
> >version.
> +	 *
> +	 * Note: The ioctl argument is extended and zeropadded by core
> DRM.
>  	 */
> -	if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
> -		     arg.version == 0)) {
> +	if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
> +		     arg->version == 0)) {
>  		VMW_DEBUG_USER("Incorrect execbuf version.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (arg.version > 1 &&
> -	    copy_from_user(&arg.context_handle,
> -			   (void __user *) (data + copy_offset[0]),
> -			   copy_offset[arg.version - 1] -
> copy_offset[0]) != 0)
> -		return -EFAULT;
> -
> -	switch (arg.version) {
> +	switch (arg->version) {
>  	case 1:
> -		arg.context_handle = (uint32_t) -1;
> +		/* For v1 core DRM have extended + zeropadded the data
> */
> +		arg->context_handle = (uint32_t) -1;
>  		break;
>  	case 2:
>  	default:
> +		/* For v2 and later core DRM would have correctly
> copied it */
>  		break;
>  	}
>  
>  	/* If imported a fence FD from elsewhere, then wait on it */
> -	if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> -		in_fence = sync_file_get_fence(arg.imported_fence_fd);
> +	if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> +		in_fence = sync_file_get_fence(arg->imported_fence_fd);
>  
>  		if (!in_fence) {
>  			VMW_DEBUG_USER("Cannot get imported fence\n");
> @@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device *dev,
> unsigned long data,
>  		return ret;
>  
>  	ret = vmw_execbuf_process(file_priv, dev_priv,
> -				  (void __user *)(unsigned
> long)arg.commands,
> -				  NULL, arg.command_size,
> arg.throttle_us,
> -				  arg.context_handle,
> -				  (void __user *)(unsigned
> long)arg.fence_rep,
> -				  NULL, arg.flags);
> +				  (void __user *)(unsigned long)arg-
> >commands,
> +				  NULL, arg->command_size, arg-
> >throttle_us,
> +				  arg->context_handle,
> +				  (void __user *)(unsigned long)arg-
> >fence_rep,
> +				  NULL, arg->flags);
>  
>  	ttm_read_unlock(&dev_priv->reservation_sem);
>  	if (unlikely(ret != 0))
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Thomas Hellstrom]

On Wed, 2019-05-22 at 21:09 +0200, Daniel Vetter wrote:
> On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <
> thellstrom@vmware.com> wrote:
> > Hi, Emil,
> > 
> > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > > 
> > > Currently vmw_execbuf_ioctl() open-codes the permission checking,
> > > size
> > > extending and copying that is already done in core drm.
> > > 
> > > Kill all the duplication, adding a few comments for clarity.
> > 
> > Ah, there is core functionality for this now.
> > 
> > What worries me though with the core approach is that the sizes are
> > not
> > capped by the size of the kernel argument definition, which makes
> > mailicious user-space being able to force kmallocs() the size of
> > the
> > maximum ioctl size. Should probably be fixed before pushing this.
> 
> Hm I always worked under the assumption that kmalloc and friends
> should be userspace hardened. Otherwise stuff like kmalloc_array
> doesn't make any sense, everyone just feeds it unchecked input and
> expects that helper to handle overflows.
> 
> If we assume kmalloc isn't hardened against that, then we have a much
> bigger problem than just vmwgfx ioctls ...

After checking the drm_ioctl code I realize that what I thought was new
behaviour actually has been around for a couple of years, so
fixing isn't really tied to this patch series...

What caused me to react was that previously we used to have this

e4fda9f264e1 ("drm: Perform ioctl command validation on the stored
kernel values")

and we seem to have lost that now, if not for the io flags then at
least for the size part. For the size of the ioctl arguments, I think
in general if the kernel only touches a subset of the user-space
specified size I see no reason why we should malloc / copy more than
that?

Now, given the fact that the maximum ioctl argument size is quite
limited, that might not be a big problem or a problem at all. Otherwise
it would be pretty easy for a malicious process to allocate most or all
of a system's resident memory?

/Thomas







> -Daniel
> 
> > 
> > > Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> > > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > ---
> > > Thomas, VMware team,
> > > 
> > > Please give this some testing on your end. I've only tested it
> > > against
> > > mesa-master.
> > 
> > I'll review tomorrow and do some testing. Need to see if I can dig
> > up
> > user-space apps with version 0...
> > 
> > Thanks,
> > 
> > Thomas
> > 
> > > Thanks
> > > Emil
> > > ---
> > >  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 12 +-----
> > >  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  4 +-
> > >  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++--------
> > > ----
> > > ----
> > >  3 files changed, 23 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > index d3f108f7e52d..2cb6ae219e43 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > @@ -186,7 +186,7 @@ static const struct drm_ioctl_desc
> > > vmw_ioctls[] =
> > > {
> > >                     DRM_RENDER_ALLOW),
> > >       VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
> > >                     DRM_AUTH | DRM_RENDER_ALLOW),
> > > -     VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
> > > +     VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
> > >                     DRM_RENDER_ALLOW),
> > >       VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
> > >                     DRM_RENDER_ALLOW),
> > > @@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file
> > > *filp, unsigned int cmd,
> > >                       &vmw_ioctls[nr - DRM_COMMAND_BASE];
> > > 
> > >               if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
> > > -                     ret = (long) drm_ioctl_permit(ioctl->flags,
> > > file_priv);
> > > -                     if (unlikely(ret != 0))
> > > -                             return ret;
> > > -
> > > -                     if (unlikely((cmd & (IOC_IN | IOC_OUT)) !=
> > > IOC_IN))
> > > -                             goto out_io_encoding;
> > > -
> > > -                     return (long) vmw_execbuf_ioctl(dev, arg,
> > > file_priv,
> > > -                                                     _IOC_SIZE(c
> > > md))
> > > ;
> > > +                     return ioctl_func(filp, cmd, arg);
> > >               } else if (nr == DRM_COMMAND_BASE +
> > > DRM_VMW_UPDATE_LAYOUT) {
> > >                       if (!drm_is_current_master(file_priv) &&
> > >                           !capable(CAP_SYS_ADMIN))
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > index 9be2176cc260..f5bfac85f793 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > @@ -910,8 +910,8 @@ static inline struct page
> > > *vmw_piter_page(struct
> > > vmw_piter *viter)
> > >   * Command submission - vmwgfx_execbuf.c
> > >   */
> > > 
> > > -extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned
> > > long
> > > data,
> > > -                          struct drm_file *file_priv, size_t
> > > size);
> > > +extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> > > +                          struct drm_file *file_priv);
> > >  extern int vmw_execbuf_process(struct drm_file *file_priv,
> > >                              struct vmw_private *dev_priv,
> > >                              void __user *user_commands,
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > index 2ff7ba04d8c8..767e2b99618d 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > @@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct
> > > vmw_private *dev_priv)
> > >       mutex_unlock(&dev_priv->cmdbuf_mutex);
> > >  }
> > > 
> > > -int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long
> > > data,
> > > -                   struct drm_file *file_priv, size_t size)
> > > +int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> > > +                   struct drm_file *file_priv)
> > >  {
> > >       struct vmw_private *dev_priv = vmw_priv(dev);
> > > -     struct drm_vmw_execbuf_arg arg;
> > > +     struct drm_vmw_execbuf_arg *arg = data;
> > >       int ret;
> > > -     static const size_t copy_offset[] = {
> > > -             offsetof(struct drm_vmw_execbuf_arg,
> > > context_handle),
> > > -             sizeof(struct drm_vmw_execbuf_arg)};
> > >       struct dma_fence *in_fence = NULL;
> > > 
> > > -     if (unlikely(size < copy_offset[0])) {
> > > -             VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
> > > -                            DRM_VMW_EXECBUF);
> > > -             return -EINVAL;
> > > -     }
> > > -
> > > -     if (copy_from_user(&arg, (void __user *) data,
> > > copy_offset[0])
> > > != 0)
> > > -             return -EFAULT;
> > > -
> > >       /*
> > >        * Extend the ioctl argument while maintaining backwards
> > > compatibility:
> > > -      * We take different code paths depending on the value of
> > > arg.version.
> > > +      * We take different code paths depending on the value of
> > > arg-
> > > > version.
> > > +      *
> > > +      * Note: The ioctl argument is extended and zeropadded by
> > > core
> > > DRM.
> > >        */
> > > -     if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
> > > -                  arg.version == 0)) {
> > > +     if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
> > > +                  arg->version == 0)) {
> > >               VMW_DEBUG_USER("Incorrect execbuf version.\n");
> > >               return -EINVAL;
> > >       }
> > > 
> > > -     if (arg.version > 1 &&
> > > -         copy_from_user(&arg.context_handle,
> > > -                        (void __user *) (data + copy_offset[0]),
> > > -                        copy_offset[arg.version - 1] -
> > > copy_offset[0]) != 0)
> > > -             return -EFAULT;
> > > -
> > > -     switch (arg.version) {
> > > +     switch (arg->version) {
> > >       case 1:
> > > -             arg.context_handle = (uint32_t) -1;
> > > +             /* For v1 core DRM have extended + zeropadded the
> > > data
> > > */
> > > +             arg->context_handle = (uint32_t) -1;
> > >               break;
> > >       case 2:
> > >       default:
> > > +             /* For v2 and later core DRM would have correctly
> > > copied it */
> > >               break;
> > >       }
> > > 
> > >       /* If imported a fence FD from elsewhere, then wait on it
> > > */
> > > -     if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> > > -             in_fence =
> > > sync_file_get_fence(arg.imported_fence_fd);
> > > +     if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> > > +             in_fence = sync_file_get_fence(arg-
> > > >imported_fence_fd);
> > > 
> > >               if (!in_fence) {
> > >                       VMW_DEBUG_USER("Cannot get imported
> > > fence\n");
> > > @@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device
> > > *dev,
> > > unsigned long data,
> > >               return ret;
> > > 
> > >       ret = vmw_execbuf_process(file_priv, dev_priv,
> > > -                               (void __user *)(unsigned
> > > long)arg.commands,
> > > -                               NULL, arg.command_size,
> > > arg.throttle_us,
> > > -                               arg.context_handle,
> > > -                               (void __user *)(unsigned
> > > long)arg.fence_rep,
> > > -                               NULL, arg.flags);
> > > +                               (void __user *)(unsigned
> > > long)arg-
> > > > commands,
> > > +                               NULL, arg->command_size, arg-
> > > > throttle_us,
> > > +                               arg->context_handle,
> > > +                               (void __user *)(unsigned
> > > long)arg-
> > > > fence_rep,
> > > +                               NULL, arg->flags);
> > > 
> > >       ttm_read_unlock(&dev_priv->reservation_sem);
> > >       if (unlikely(ret != 0))
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Daniel Vetter]

On Fri, May 24, 2019 at 8:05 AM Thomas Hellstrom <thellstrom@vmware.com> wrote:
>
> On Wed, 2019-05-22 at 21:09 +0200, Daniel Vetter wrote:
> > On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <
> > thellstrom@vmware.com> wrote:
> > > Hi, Emil,
> > >
> > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > >
> > > > Currently vmw_execbuf_ioctl() open-codes the permission checking,
> > > > size
> > > > extending and copying that is already done in core drm.
> > > >
> > > > Kill all the duplication, adding a few comments for clarity.
> > >
> > > Ah, there is core functionality for this now.
> > >
> > > What worries me though with the core approach is that the sizes are
> > > not
> > > capped by the size of the kernel argument definition, which makes
> > > mailicious user-space being able to force kmallocs() the size of
> > > the
> > > maximum ioctl size. Should probably be fixed before pushing this.
> >
> > Hm I always worked under the assumption that kmalloc and friends
> > should be userspace hardened. Otherwise stuff like kmalloc_array
> > doesn't make any sense, everyone just feeds it unchecked input and
> > expects that helper to handle overflows.
> >
> > If we assume kmalloc isn't hardened against that, then we have a much
> > bigger problem than just vmwgfx ioctls ...
>
> After checking the drm_ioctl code I realize that what I thought was new
> behaviour actually has been around for a couple of years, so
> fixing isn't really tied to this patch series...
>
> What caused me to react was that previously we used to have this
>
> e4fda9f264e1 ("drm: Perform ioctl command validation on the stored
> kernel values")
>
> and we seem to have lost that now, if not for the io flags then at
> least for the size part. For the size of the ioctl arguments, I think
> in general if the kernel only touches a subset of the user-space
> specified size I see no reason why we should malloc / copy more than
> that?

I guess we could optimize that, but we'd probably still need to zero
clear the added size for forward compat with newer userspace. Iirc
we've had some issues in this area.

> Now, given the fact that the maximum ioctl argument size is quite
> limited, that might not be a big problem or a problem at all. Otherwise
> it would be pretty easy for a malicious process to allocate most or all
> of a system's resident memory?

The biggest you can allocate from kmalloc is limited by the largest
contiguous chunk alloc_pages gives you, which is limited by MAX_ORDER
from the page buddy allocator. You need lots of process to be able to
exhaust memory like that (and like I said, the entire kernel would be
broken if we'd consider this a security issue). If you want to make
sure that a process group can't exhaust memory this way then you need
to set appropriate cgroups limits.
-Daniel

>
> /Thomas
>
>
>
>
>
>
>
> > -Daniel
> >
> > >
> > > > Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> > > > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > > ---
> > > > Thomas, VMware team,
> > > >
> > > > Please give this some testing on your end. I've only tested it
> > > > against
> > > > mesa-master.
> > >
> > > I'll review tomorrow and do some testing. Need to see if I can dig
> > > up
> > > user-space apps with version 0...
> > >
> > > Thanks,
> > >
> > > Thomas
> > >
> > > > Thanks
> > > > Emil
> > > > ---
> > > >  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 12 +-----
> > > >  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  4 +-
> > > >  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++--------
> > > > ----
> > > > ----
> > > >  3 files changed, 23 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > > index d3f108f7e52d..2cb6ae219e43 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > > @@ -186,7 +186,7 @@ static const struct drm_ioctl_desc
> > > > vmw_ioctls[] =
> > > > {
> > > >                     DRM_RENDER_ALLOW),
> > > >       VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
> > > >                     DRM_AUTH | DRM_RENDER_ALLOW),
> > > > -     VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
> > > > +     VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
> > > >                     DRM_RENDER_ALLOW),
> > > >       VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
> > > >                     DRM_RENDER_ALLOW),
> > > > @@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file
> > > > *filp, unsigned int cmd,
> > > >                       &vmw_ioctls[nr - DRM_COMMAND_BASE];
> > > >
> > > >               if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
> > > > -                     ret = (long) drm_ioctl_permit(ioctl->flags,
> > > > file_priv);
> > > > -                     if (unlikely(ret != 0))
> > > > -                             return ret;
> > > > -
> > > > -                     if (unlikely((cmd & (IOC_IN | IOC_OUT)) !=
> > > > IOC_IN))
> > > > -                             goto out_io_encoding;
> > > > -
> > > > -                     return (long) vmw_execbuf_ioctl(dev, arg,
> > > > file_priv,
> > > > -                                                     _IOC_SIZE(c
> > > > md))
> > > > ;
> > > > +                     return ioctl_func(filp, cmd, arg);
> > > >               } else if (nr == DRM_COMMAND_BASE +
> > > > DRM_VMW_UPDATE_LAYOUT) {
> > > >                       if (!drm_is_current_master(file_priv) &&
> > > >                           !capable(CAP_SYS_ADMIN))
> > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > > index 9be2176cc260..f5bfac85f793 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > > @@ -910,8 +910,8 @@ static inline struct page
> > > > *vmw_piter_page(struct
> > > > vmw_piter *viter)
> > > >   * Command submission - vmwgfx_execbuf.c
> > > >   */
> > > >
> > > > -extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned
> > > > long
> > > > data,
> > > > -                          struct drm_file *file_priv, size_t
> > > > size);
> > > > +extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> > > > +                          struct drm_file *file_priv);
> > > >  extern int vmw_execbuf_process(struct drm_file *file_priv,
> > > >                              struct vmw_private *dev_priv,
> > > >                              void __user *user_commands,
> > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > > index 2ff7ba04d8c8..767e2b99618d 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > > @@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct
> > > > vmw_private *dev_priv)
> > > >       mutex_unlock(&dev_priv->cmdbuf_mutex);
> > > >  }
> > > >
> > > > -int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long
> > > > data,
> > > > -                   struct drm_file *file_priv, size_t size)
> > > > +int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> > > > +                   struct drm_file *file_priv)
> > > >  {
> > > >       struct vmw_private *dev_priv = vmw_priv(dev);
> > > > -     struct drm_vmw_execbuf_arg arg;
> > > > +     struct drm_vmw_execbuf_arg *arg = data;
> > > >       int ret;
> > > > -     static const size_t copy_offset[] = {
> > > > -             offsetof(struct drm_vmw_execbuf_arg,
> > > > context_handle),
> > > > -             sizeof(struct drm_vmw_execbuf_arg)};
> > > >       struct dma_fence *in_fence = NULL;
> > > >
> > > > -     if (unlikely(size < copy_offset[0])) {
> > > > -             VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
> > > > -                            DRM_VMW_EXECBUF);
> > > > -             return -EINVAL;
> > > > -     }
> > > > -
> > > > -     if (copy_from_user(&arg, (void __user *) data,
> > > > copy_offset[0])
> > > > != 0)
> > > > -             return -EFAULT;
> > > > -
> > > >       /*
> > > >        * Extend the ioctl argument while maintaining backwards
> > > > compatibility:
> > > > -      * We take different code paths depending on the value of
> > > > arg.version.
> > > > +      * We take different code paths depending on the value of
> > > > arg-
> > > > > version.
> > > > +      *
> > > > +      * Note: The ioctl argument is extended and zeropadded by
> > > > core
> > > > DRM.
> > > >        */
> > > > -     if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
> > > > -                  arg.version == 0)) {
> > > > +     if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
> > > > +                  arg->version == 0)) {
> > > >               VMW_DEBUG_USER("Incorrect execbuf version.\n");
> > > >               return -EINVAL;
> > > >       }
> > > >
> > > > -     if (arg.version > 1 &&
> > > > -         copy_from_user(&arg.context_handle,
> > > > -                        (void __user *) (data + copy_offset[0]),
> > > > -                        copy_offset[arg.version - 1] -
> > > > copy_offset[0]) != 0)
> > > > -             return -EFAULT;
> > > > -
> > > > -     switch (arg.version) {
> > > > +     switch (arg->version) {
> > > >       case 1:
> > > > -             arg.context_handle = (uint32_t) -1;
> > > > +             /* For v1 core DRM have extended + zeropadded the
> > > > data
> > > > */
> > > > +             arg->context_handle = (uint32_t) -1;
> > > >               break;
> > > >       case 2:
> > > >       default:
> > > > +             /* For v2 and later core DRM would have correctly
> > > > copied it */
> > > >               break;
> > > >       }
> > > >
> > > >       /* If imported a fence FD from elsewhere, then wait on it
> > > > */
> > > > -     if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> > > > -             in_fence =
> > > > sync_file_get_fence(arg.imported_fence_fd);
> > > > +     if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> > > > +             in_fence = sync_file_get_fence(arg-
> > > > >imported_fence_fd);
> > > >
> > > >               if (!in_fence) {
> > > >                       VMW_DEBUG_USER("Cannot get imported
> > > > fence\n");
> > > > @@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device
> > > > *dev,
> > > > unsigned long data,
> > > >               return ret;
> > > >
> > > >       ret = vmw_execbuf_process(file_priv, dev_priv,
> > > > -                               (void __user *)(unsigned
> > > > long)arg.commands,
> > > > -                               NULL, arg.command_size,
> > > > arg.throttle_us,
> > > > -                               arg.context_handle,
> > > > -                               (void __user *)(unsigned
> > > > long)arg.fence_rep,
> > > > -                               NULL, arg.flags);
> > > > +                               (void __user *)(unsigned
> > > > long)arg-
> > > > > commands,
> > > > +                               NULL, arg->command_size, arg-
> > > > > throttle_us,
> > > > +                               arg->context_handle,
> > > > +                               (void __user *)(unsigned
> > > > long)arg-
> > > > > fence_rep,
> > > > +                               NULL, arg->flags);
> > > >
> > > >       ttm_read_unlock(&dev_priv->reservation_sem);
> > > >       if (unlikely(ret != 0))
> >
> >



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

Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Emil Velikov]

On 2019/05/24, Daniel Vetter wrote:
> On Fri, May 24, 2019 at 8:05 AM Thomas Hellstrom <thellstrom@vmware.com> wrote:
> >
> > On Wed, 2019-05-22 at 21:09 +0200, Daniel Vetter wrote:
> > > On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <
> > > thellstrom@vmware.com> wrote:
> > > > Hi, Emil,
> > > >
> > > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > >
> > > > > Currently vmw_execbuf_ioctl() open-codes the permission checking,
> > > > > size
> > > > > extending and copying that is already done in core drm.
> > > > >
> > > > > Kill all the duplication, adding a few comments for clarity.
> > > >
> > > > Ah, there is core functionality for this now.
> > > >
> > > > What worries me though with the core approach is that the sizes are
> > > > not
> > > > capped by the size of the kernel argument definition, which makes
> > > > mailicious user-space being able to force kmallocs() the size of
> > > > the
> > > > maximum ioctl size. Should probably be fixed before pushing this.
> > >
> > > Hm I always worked under the assumption that kmalloc and friends
> > > should be userspace hardened. Otherwise stuff like kmalloc_array
> > > doesn't make any sense, everyone just feeds it unchecked input and
> > > expects that helper to handle overflows.
> > >
> > > If we assume kmalloc isn't hardened against that, then we have a much
> > > bigger problem than just vmwgfx ioctls ...
> >
> > After checking the drm_ioctl code I realize that what I thought was new
> > behaviour actually has been around for a couple of years, so
> > fixing isn't really tied to this patch series...
> >
> > What caused me to react was that previously we used to have this
> >
> > e4fda9f264e1 ("drm: Perform ioctl command validation on the stored
> > kernel values")
> >
> > and we seem to have lost that now, if not for the io flags then at
> > least for the size part. For the size of the ioctl arguments, I think
> > in general if the kernel only touches a subset of the user-space
> > specified size I see no reason why we should malloc / copy more than
> > that?
> 
> I guess we could optimize that, but we'd probably still need to zero
> clear the added size for forward compat with newer userspace. Iirc
> we've had some issues in this area.
> 
> > Now, given the fact that the maximum ioctl argument size is quite
> > limited, that might not be a big problem or a problem at all. Otherwise
> > it would be pretty easy for a malicious process to allocate most or all
> > of a system's resident memory?
> 
> The biggest you can allocate from kmalloc is limited by the largest
> contiguous chunk alloc_pages gives you, which is limited by MAX_ORDER
> from the page buddy allocator. You need lots of process to be able to
> exhaust memory like that (and like I said, the entire kernel would be
> broken if we'd consider this a security issue). If you want to make
> sure that a process group can't exhaust memory this way then you need
> to set appropriate cgroups limits.

I do agree with all the sentiments that drm_ioctl() could use some extra
optimisation and hardening. At the same time I would remind that the
code has been used as-is by vmwgfx and other drivers for years.

In other words: let's keep that work as orthogonal series.

What do you guys think?
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Thomas Hellstrom]

On 5/24/19 12:53 PM, Emil Velikov wrote:
> On 2019/05/24, Daniel Vetter wrote:
>> On Fri, May 24, 2019 at 8:05 AM Thomas Hellstrom <thellstrom@vmware.com> wrote:
>>> On Wed, 2019-05-22 at 21:09 +0200, Daniel Vetter wrote:
>>>> On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <
>>>> thellstrom@vmware.com> wrote:
>>>>> Hi, Emil,
>>>>>
>>>>> On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
>>>>>> From: Emil Velikov <emil.velikov@collabora.com>
>>>>>>
>>>>>> Currently vmw_execbuf_ioctl() open-codes the permission checking,
>>>>>> size
>>>>>> extending and copying that is already done in core drm.
>>>>>>
>>>>>> Kill all the duplication, adding a few comments for clarity.
>>>>> Ah, there is core functionality for this now.
>>>>>
>>>>> What worries me though with the core approach is that the sizes are
>>>>> not
>>>>> capped by the size of the kernel argument definition, which makes
>>>>> mailicious user-space being able to force kmallocs() the size of
>>>>> the
>>>>> maximum ioctl size. Should probably be fixed before pushing this.
>>>> Hm I always worked under the assumption that kmalloc and friends
>>>> should be userspace hardened. Otherwise stuff like kmalloc_array
>>>> doesn't make any sense, everyone just feeds it unchecked input and
>>>> expects that helper to handle overflows.
>>>>
>>>> If we assume kmalloc isn't hardened against that, then we have a much
>>>> bigger problem than just vmwgfx ioctls ...
>>> After checking the drm_ioctl code I realize that what I thought was new
>>> behaviour actually has been around for a couple of years, so
>>> fixing isn't really tied to this patch series...
>>>
>>> What caused me to react was that previously we used to have this
>>>
>>> e4fda9f264e1 ("drm: Perform ioctl command validation on the stored
>>> kernel values")
>>>
>>> and we seem to have lost that now, if not for the io flags then at
>>> least for the size part. For the size of the ioctl arguments, I think
>>> in general if the kernel only touches a subset of the user-space
>>> specified size I see no reason why we should malloc / copy more than
>>> that?
>> I guess we could optimize that, but we'd probably still need to zero
>> clear the added size for forward compat with newer userspace. Iirc
>> we've had some issues in this area.
>>
>>> Now, given the fact that the maximum ioctl argument size is quite
>>> limited, that might not be a big problem or a problem at all. Otherwise
>>> it would be pretty easy for a malicious process to allocate most or all
>>> of a system's resident memory?
>> The biggest you can allocate from kmalloc is limited by the largest
>> contiguous chunk alloc_pages gives you, which is limited by MAX_ORDER
>> from the page buddy allocator. You need lots of process to be able to
>> exhaust memory like that (and like I said, the entire kernel would be
>> broken if we'd consider this a security issue). If you want to make
>> sure that a process group can't exhaust memory this way then you need
>> to set appropriate cgroups limits.
> I do agree with all the sentiments that drm_ioctl() could use some extra
> optimisation and hardening. At the same time I would remind that the
> code has been used as-is by vmwgfx and other drivers for years.
>
> In other words: let's keep that work as orthogonal series.
>
> What do you guys think?

I agree. Then I only had a concern with one of the patches.

/Thomas


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


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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

On 2019/05/23, Thomas Hellstrom wrote:
> Hi, Emil,
> 
> On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> > 
> > Drop the custom ioctl io encoding check - core drm does it for us.
> 
> I fail to see where the core does this, or do I miss something?

drm_ioctl() allows for the encoding to be changed and attributes that only the
appropriate size is copied in/out of the kernel.

Technically the function is more relaxed relative to the vmwgfx check, yet
seems perfectly reasonable.

Is there any corner-case that isn't but should be handled in drm_ioctl()?

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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom]

On Fri, 2019-05-24 at 13:14 +0100, Emil Velikov wrote:
> On 2019/05/23, Thomas Hellstrom wrote:
> > Hi, Emil,
> > 
> > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > > 
> > > Drop the custom ioctl io encoding check - core drm does it for
> > > us.
> > 
> > I fail to see where the core does this, or do I miss something?
> 
> drm_ioctl() allows for the encoding to be changed and attributes that
> only the
> appropriate size is copied in/out of the kernel.
> 
> Technically the function is more relaxed relative to the vmwgfx
> check, yet
> seems perfectly reasonable.
> 
> Is there any corner-case that isn't but should be handled in
> drm_ioctl()?

I'd like to turn the question around and ask whether there's a reason
we should relax the vmwgfx test? In the past it has trapped quite a few
user-space errors.

Thanks,
Thomas



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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

On 2019/05/24, Thomas Hellstrom wrote:
> On Fri, 2019-05-24 at 13:14 +0100, Emil Velikov wrote:
> > On 2019/05/23, Thomas Hellstrom wrote:
> > > Hi, Emil,
> > > 
> > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > 
> > > > Drop the custom ioctl io encoding check - core drm does it for
> > > > us.
> > > 
> > > I fail to see where the core does this, or do I miss something?
> > 
> > drm_ioctl() allows for the encoding to be changed and attributes that
> > only the
> > appropriate size is copied in/out of the kernel.
> > 
> > Technically the function is more relaxed relative to the vmwgfx
> > check, yet
> > seems perfectly reasonable.
> > 
> > Is there any corner-case that isn't but should be handled in
> > drm_ioctl()?
> 
> I'd like to turn the question around and ask whether there's a reason
> we should relax the vmwgfx test? In the past it has trapped quite a few
> user-space errors.
> 
The way I see it either:
 - the check, as-is, is unnessesary, or
 - it is needed, and we should do something equivalent for all of DRM

We had a very long brainstorming session with a colleague and we could not see
any cases where this would cause a problem. If you recall anything concrete
please let me know - I would be more than happy to take a closer look.

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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom]

Hi, Emil

On Fri, 2019-05-24 at 16:26 +0100, Emil Velikov wrote:
> On 2019/05/24, Thomas Hellstrom wrote:
> > On Fri, 2019-05-24 at 13:14 +0100, Emil Velikov wrote:
> > > On 2019/05/23, Thomas Hellstrom wrote:
> > > > Hi, Emil,
> > > > 
> > > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > > 
> > > > > Drop the custom ioctl io encoding check - core drm does it
> > > > > for
> > > > > us.
> > > > 
> > > > I fail to see where the core does this, or do I miss something?
> > > 
> > > drm_ioctl() allows for the encoding to be changed and attributes
> > > that
> > > only the
> > > appropriate size is copied in/out of the kernel.
> > > 
> > > Technically the function is more relaxed relative to the vmwgfx
> > > check, yet
> > > seems perfectly reasonable.
> > > 
> > > Is there any corner-case that isn't but should be handled in
> > > drm_ioctl()?
> > 
> > I'd like to turn the question around and ask whether there's a
> > reason
> > we should relax the vmwgfx test? In the past it has trapped quite a
> > few
> > user-space errors.
> > 
> The way I see it either:
>  - the check, as-is, is unnessesary, or
>  - it is needed, and we should do something equivalent for all of DRM
> 
> We had a very long brainstorming session with a colleague and we
> could not see
> any cases where this would cause a problem. If you recall anything
> concrete
> please let me know - I would be more than happy to take a closer
> look.

If you have a good reason to drop an ioctl sanity check, I'd be
perfectly happy to do it. To me, a good reason even includes "I have a
non-open-source customer having problems with this check" because of
reason etc. etc. as long as I have a way to evaluate those reasons and
determine if they're valid or not. "No other drm driver nor the core is
doing this" is NOT a valid reason to me. In particular if the check is
not affecting performance. So unless you provide additional reasons to
drop this check, it's a solid NAK from my side.

Thanks,
Thomas


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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom]

On Sat, 2019-05-25 at 00:39 +0200, Thomas Hellström wrote:
> Hi, Emil
> 
> On Fri, 2019-05-24 at 16:26 +0100, Emil Velikov wrote:
> > On 2019/05/24, Thomas Hellstrom wrote:
> > > On Fri, 2019-05-24 at 13:14 +0100, Emil Velikov wrote:
> > > > On 2019/05/23, Thomas Hellstrom wrote:
> > > > > Hi, Emil,
> > > > > 
> > > > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > > > 
> > > > > > Drop the custom ioctl io encoding check - core drm does it
> > > > > > for
> > > > > > us.
> > > > > 
> > > > > I fail to see where the core does this, or do I miss
> > > > > something?
> > > > 
> > > > drm_ioctl() allows for the encoding to be changed and
> > > > attributes
> > > > that
> > > > only the
> > > > appropriate size is copied in/out of the kernel.
> > > > 
> > > > Technically the function is more relaxed relative to the vmwgfx
> > > > check, yet
> > > > seems perfectly reasonable.
> > > > 
> > > > Is there any corner-case that isn't but should be handled in
> > > > drm_ioctl()?
> > > 
> > > I'd like to turn the question around and ask whether there's a
> > > reason
> > > we should relax the vmwgfx test? In the past it has trapped quite
> > > a
> > > few
> > > user-space errors.
> > > 
> > The way I see it either:
> >  - the check, as-is, is unnessesary, or
> >  - it is needed, and we should do something equivalent for all of
> > DRM
> > 
> > We had a very long brainstorming session with a colleague and we
> > could not see
> > any cases where this would cause a problem. If you recall anything
> > concrete
> > please let me know - I would be more than happy to take a closer
> > look.
> 
> If you have a good reason to drop an ioctl sanity check, I'd be
> perfectly happy to do it. To me, a good reason even includes "I have
> a
> non-open-source customer having problems with this check" because of
> reason etc. etc. as long as I have a way to evaluate those reasons
> and
> determine if they're valid or not. "No other drm driver nor the core
> is
> doing this" is NOT a valid reason to me. In particular if the check
> is
> not affecting performance. So unless you provide additional reasons
> to
> drop this check, it's a solid NAK from my side.

To clarify my point of view a bit, this check is useful to early catch
userspace using incorrect flags and sizes, which otherwise might make
it out to distros and after that, introducing a check like this would
be impossible, since it might break old user-space. For the same reason
it would probably be very difficult to introduce it in core drm. 

Thanks,
Thomas



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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

On 2019/05/25, Thomas Hellstrom wrote:
> On Sat, 2019-05-25 at 00:39 +0200, Thomas Hellström wrote:
> > Hi, Emil
> > 
> > On Fri, 2019-05-24 at 16:26 +0100, Emil Velikov wrote:
> > > On 2019/05/24, Thomas Hellstrom wrote:
> > > > On Fri, 2019-05-24 at 13:14 +0100, Emil Velikov wrote:
> > > > > On 2019/05/23, Thomas Hellstrom wrote:
> > > > > > Hi, Emil,
> > > > > > 
> > > > > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > > > > 
> > > > > > > Drop the custom ioctl io encoding check - core drm does it
> > > > > > > for
> > > > > > > us.
> > > > > > 
> > > > > > I fail to see where the core does this, or do I miss
> > > > > > something?
> > > > > 
> > > > > drm_ioctl() allows for the encoding to be changed and
> > > > > attributes
> > > > > that
> > > > > only the
> > > > > appropriate size is copied in/out of the kernel.
> > > > > 
> > > > > Technically the function is more relaxed relative to the vmwgfx
> > > > > check, yet
> > > > > seems perfectly reasonable.
> > > > > 
> > > > > Is there any corner-case that isn't but should be handled in
> > > > > drm_ioctl()?
> > > > 
> > > > I'd like to turn the question around and ask whether there's a
> > > > reason
> > > > we should relax the vmwgfx test? In the past it has trapped quite
> > > > a
> > > > few
> > > > user-space errors.
> > > > 
> > > The way I see it either:
> > >  - the check, as-is, is unnessesary, or
> > >  - it is needed, and we should do something equivalent for all of
> > > DRM
> > > 
> > > We had a very long brainstorming session with a colleague and we
> > > could not see
> > > any cases where this would cause a problem. If you recall anything
> > > concrete
> > > please let me know - I would be more than happy to take a closer
> > > look.
> > 
> > If you have a good reason to drop an ioctl sanity check, I'd be
> > perfectly happy to do it. To me, a good reason even includes "I have
> > a
> > non-open-source customer having problems with this check" because of
> > reason etc. etc. as long as I have a way to evaluate those reasons
> > and
> > determine if they're valid or not. "No other drm driver nor the core
> > is
> > doing this" is NOT a valid reason to me. In particular if the check
> > is
> > not affecting performance. So unless you provide additional reasons
> > to
> > drop this check, it's a solid NAK from my side.
> 
> To clarify my point of view a bit, this check is useful to early catch
> userspace using incorrect flags and sizes, which otherwise might make
> it out to distros and after that, introducing a check like this would
> be impossible, since it might break old user-space. For the same reason
> it would probably be very difficult to introduce it in core drm. 
> 
I think we might be talking past each other, let's take a step back:

 - as of previous patch, all of vmwgfx ioctls size is consistently
handled by the core
 - handling of featue flags, as always, is responsibility of the driver
ifself
 - with this patch, ioctl direction is also handled by core.

Here core ensures we only copy in/out as much data as the kernel
implementation can handle.


Let's consider the following real world example - msm and virtio_gpu.

An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
 - we add a flag to annotate/request the out, as always invalid flags
are return -EINVAL
 - we change the ioctl encoding

As currently handled by core DRM, old kernel/new userspace and
vice-versa works just fine. Sadly, vmwgfx will error out, while it could
be avoided.

As said above, I'll gladly adjust core and/or others, if this relaxed
approach causes an issue somewhere. A specific use-case, real or
hypothetical will be appreciated.


All this is part of an "evil" plan of mine, to port cool features from
vmwgfx to core and effectively remove the vmw_generic_ioctl() wrapper.


Hope the bigger picture is clearer now, if not please let me know.

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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom]

Hi, Emil,

On Mon, 2019-05-27 at 10:08 +0100, Emil Velikov wrote:
> On 2019/05/25, Thomas Hellstrom wrote:
> > On Sat, 2019-05-25 at 00:39 +0200, Thomas Hellström wrote:
> > > Hi, Emil
> > > 
> > > On Fri, 2019-05-24 at 16:26 +0100, Emil Velikov wrote:
> > > > On 2019/05/24, Thomas Hellstrom wrote:
> > > > > On Fri, 2019-05-24 at 13:14 +0100, Emil Velikov wrote:
> > > > > > On 2019/05/23, Thomas Hellstrom wrote:
> > > > > > > Hi, Emil,
> > > > > > > 
> > > > > > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > > > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > > > > > 
> > > > > > > > Drop the custom ioctl io encoding check - core drm does
> > > > > > > > it
> > > > > > > > for
> > > > > > > > us.
> > > > > > > 
> > > > > > > I fail to see where the core does this, or do I miss
> > > > > > > something?
> > > > > > 
> > > > > > drm_ioctl() allows for the encoding to be changed and
> > > > > > attributes
> > > > > > that
> > > > > > only the
> > > > > > appropriate size is copied in/out of the kernel.
> > > > > > 
> > > > > > Technically the function is more relaxed relative to the
> > > > > > vmwgfx
> > > > > > check, yet
> > > > > > seems perfectly reasonable.
> > > > > > 
> > > > > > Is there any corner-case that isn't but should be handled
> > > > > > in
> > > > > > drm_ioctl()?
> > > > > 
> > > > > I'd like to turn the question around and ask whether there's
> > > > > a
> > > > > reason
> > > > > we should relax the vmwgfx test? In the past it has trapped
> > > > > quite
> > > > > a
> > > > > few
> > > > > user-space errors.
> > > > > 
> > > > The way I see it either:
> > > >  - the check, as-is, is unnessesary, or
> > > >  - it is needed, and we should do something equivalent for all
> > > > of
> > > > DRM
> > > > 
> > > > We had a very long brainstorming session with a colleague and
> > > > we
> > > > could not see
> > > > any cases where this would cause a problem. If you recall
> > > > anything
> > > > concrete
> > > > please let me know - I would be more than happy to take a
> > > > closer
> > > > look.
> > > 
> > > If you have a good reason to drop an ioctl sanity check, I'd be
> > > perfectly happy to do it. To me, a good reason even includes "I
> > > have
> > > a
> > > non-open-source customer having problems with this check" because
> > > of
> > > reason etc. etc. as long as I have a way to evaluate those
> > > reasons
> > > and
> > > determine if they're valid or not. "No other drm driver nor the
> > > core
> > > is
> > > doing this" is NOT a valid reason to me. In particular if the
> > > check
> > > is
> > > not affecting performance. So unless you provide additional
> > > reasons
> > > to
> > > drop this check, it's a solid NAK from my side.
> > 
> > To clarify my point of view a bit, this check is useful to early
> > catch
> > userspace using incorrect flags and sizes, which otherwise might
> > make
> > it out to distros and after that, introducing a check like this
> > would
> > be impossible, since it might break old user-space. For the same
> > reason
> > it would probably be very difficult to introduce it in core drm. 
> > 
> I think we might be talking past each other, let's take a step back:
> 
>  - as of previous patch, all of vmwgfx ioctls size is consistently
> handled by the core

I don't think I follow you here, AFAICT patch 3/5 only affects and
relaxes the execbuf checking (and in fact a little more than I would
like)?

>  - handling of featue flags, as always, is responsibility of the
> driver
> ifself
>  - with this patch, ioctl direction is also handled by core.
> 
> Here core ensures we only copy in/out as much data as the kernel
> implementation can handle.
> 
> 
> Let's consider the following real world example - msm and virtio_gpu.
> 
> An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
>  - we add a flag to annotate/request the out, as always invalid flags
> are return -EINVAL
>  - we change the ioctl encoding
> 
> As currently handled by core DRM, old kernel/new userspace and
> vice-versa works just fine. Sadly, vmwgfx will error out, while it
> could
> be avoided.

IMO basically we have a tradeoff between strict checking in this case,
and new user-space vs old kernel "hazzle-free" transition in the
relaxed case. 

> 
> As said above, I'll gladly adjust core and/or others, if this relaxed
> approach causes an issue somewhere. A specific use-case, real or
> hypothetical will be appreciated.

To me there are two important reasons to keep the strict approach.

1) Avoid user-space mistakes early in the development cycle. We can't
distinguish between buggy user-space and "new" user-space. This is
important because of [a]) below.

2) Catch a lot of fuzzer combinations and error out early instead of
forwarding them to the ioctl function where they may cause harm.

I think the new user-space vs old kernel can be handled nicely in user-
space with feature flags or API versions. That's the way we've handled
them up to now?

[a] But you probably can't move the stricter approach to core drm, or
even to core drm ioctls, since that may break old user-space we might
not even know about. Relaxing this now may put the vmwgfx-specific
ioctls in the same situation. I'd like to have this check also in core
drm, but alas, I think it's impossible.

> 
> 
> All this is part of an "evil" plan of mine, to port cool features
> from
> vmwgfx to core and effectively remove the vmw_generic_ioctl()
> wrapper.

I'm not completely sure I understand what role the vmw_generic_ioctl()
wrapper plays in this? I mean, the advantage of a helper approach
instead of a middle layer approach is that you are free to use the
helpers and amend them if desirable. If everybody is forced to use
exactly the same helpers, then we're basically back at the middle layer
approach?

/Thomas

> 
> 
> Hope the bigger picture is clearer now, if not please let me know.
> 
> Thanks
> Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

Hi Thomas,

On 2019/05/27, Thomas Hellstrom wrote:

> > I think we might be talking past each other, let's take a step back:
> > 
> >  - as of previous patch, all of vmwgfx ioctls size is consistently
> > handled by the core
> 
> I don't think I follow you here, AFAICT patch 3/5 only affects and
> relaxes the execbuf checking (and in fact a little more than I would
> like)?
> 
Precisely, it makes execbuf ioctl behave like all other ioctls - both
vmwgfx and rest of drm.

> >  - handling of featue flags, as always, is responsibility of the
> > driver
> > ifself
> >  - with this patch, ioctl direction is also handled by core.
> > 
> > Here core ensures we only copy in/out as much data as the kernel
> > implementation can handle.
> > 
> > 
> > Let's consider the following real world example - msm and virtio_gpu.
> > 
> > An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
> >  - we add a flag to annotate/request the out, as always invalid flags
> > are return -EINVAL
> >  - we change the ioctl encoding
> > 
> > As currently handled by core DRM, old kernel/new userspace and
> > vice-versa works just fine. Sadly, vmwgfx will error out, while it
> > could
> > be avoided.
> 
> IMO basically we have a tradeoff between strict checking in this case,
> and new user-space vs old kernel "hazzle-free" transition in the
> relaxed case. 
> 
Precisely. If I read the code correctly, ATM new userspace will fail
against old kernels. Unless userspace writes two versions of the ioctl -
with with each encoding.

> > 
> > As said above, I'll gladly adjust core and/or others, if this relaxed
> > approach causes an issue somewhere. A specific use-case, real or
> > hypothetical will be appreciated.
> 
> To me there are two important reasons to keep the strict approach.
> 
> 1) Avoid user-space mistakes early in the development cycle. We can't
> distinguish between buggy user-space and "new" user-space. This is
> important because of [a]) below.
> 
Can you provide a concrete example, please?

> 2) Catch a lot of fuzzer combinations and error out early instead of
> forwarding them to the ioctl function where they may cause harm.
> 
Struggling to see why this is a problem? At some point the fuzzer will
get past this first line of defence, so we want to make the rest of the
ioctl is robust.


> I think the new user-space vs old kernel can be handled nicely in user-
> space with feature flags or API versions. That's the way we've handled
> them up to now?
> 
How is a feature flag doing to help if the encoding changes from _IOW
to _IORW?


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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom]

On 5/27/19 2:35 PM, Emil Velikov wrote:
> Hi Thomas,
>
> On 2019/05/27, Thomas Hellstrom wrote:
>
>>> I think we might be talking past each other, let's take a step back:
>>>
>>>   - as of previous patch, all of vmwgfx ioctls size is consistently
>>> handled by the core
>> I don't think I follow you here, AFAICT patch 3/5 only affects and
>> relaxes the execbuf checking (and in fact a little more than I would
>> like)?
>>
> Precisely, it makes execbuf ioctl behave like all other ioctls - both
> vmwgfx and rest of drm.

But we're still enforcing a non-relaxed size check for the other vmwgfx 
private ioctls, right? Which is relaxed, together with the directions, 
in this commit?

(Not that it matters much to the discussion, though).

>
>>>   - handling of featue flags, as always, is responsibility of the
>>> driver
>>> ifself
>>>   - with this patch, ioctl direction is also handled by core.
>>>
>>> Here core ensures we only copy in/out as much data as the kernel
>>> implementation can handle.
>>>
>>>
>>> Let's consider the following real world example - msm and virtio_gpu.
>>>
>>> An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
>>>   - we add a flag to annotate/request the out, as always invalid flags
>>> are return -EINVAL
>>>   - we change the ioctl encoding
>>>
>>> As currently handled by core DRM, old kernel/new userspace and
>>> vice-versa works just fine. Sadly, vmwgfx will error out, while it
>>> could
>>> be avoided.
>> IMO basically we have a tradeoff between strict checking in this case,
>> and new user-space vs old kernel "hazzle-free" transition in the
>> relaxed case.
>>
> Precisely. If I read the code correctly, ATM new userspace will fail
> against old kernels. Unless userspace writes two versions of the ioctl -
> with with each encoding.
>
>>> As said above, I'll gladly adjust core and/or others, if this relaxed
>>> approach causes an issue somewhere. A specific use-case, real or
>>> hypothetical will be appreciated.
>> To me there are two important reasons to keep the strict approach.
>>
>> 1) Avoid user-space mistakes early in the development cycle. We can't
>> distinguish between buggy user-space and "new" user-space. This is
>> important because of [a]) below.
>>
> Can you provide a concrete example, please?

OK, let's say you were developing fence wait functionality. Like 
vmw_fence_obj_wait ioctl. Then suddenly you started to wonder why the 
wait never timed out as it should. The reason turn out to be that 
signals were restarting the waits with the original timeout. So you 
change the ioctl from W to RW and add a kernel-computed time to the 
argument. Everything is fine, except that you forget to change this in a 
user-space application somewhere.

So now what happens, is that that user-space bug can live on undetected 
as in 1), and that means you can never go back and implement a stricter 
check because that would completely break old user-space.

The current code will trap (and has historically trapped) code like 
this. That's mainly why I'm reluctant to give it up, but I guess it can 
be conditionally compiled in for debug purposes.

>
>> 2) Catch a lot of fuzzer combinations and error out early instead of
>> forwarding them to the ioctl function where they may cause harm.
>>
> Struggling to see why this is a problem? At some point the fuzzer will
> get past this first line of defence, so we want to make the rest of the
> ioctl is robust.
>
>
>> I think the new user-space vs old kernel can be handled nicely in user-
>> space with feature flags or API versions. That's the way we've handled
>> them up to now?
>>
> How is a feature flag doing to help if the encoding changes from _IOW
> to _IORW?

Ah, you're referring to old user-space new kernel? Yes, I was probably 
reading a bit too fast. Sorry about that.

So we're basically landing in a tradeoff between trapping problems like 
above, and hazzle-free ioctl argument definition change.

OK, so I'm ok with that as long as there is a way we can compile in 
strict checking, which will likely has to be as a vmwgfx-specific wrapper.

/Thomas


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


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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

On 2019/05/27, Thomas Hellstrom wrote:
> On 5/27/19 2:35 PM, Emil Velikov wrote:
> > Hi Thomas,
> > 
> > On 2019/05/27, Thomas Hellstrom wrote:
> > 
> > > > I think we might be talking past each other, let's take a step back:
> > > > 
> > > >   - as of previous patch, all of vmwgfx ioctls size is consistently
> > > > handled by the core
> > > I don't think I follow you here, AFAICT patch 3/5 only affects and
> > > relaxes the execbuf checking (and in fact a little more than I would
> > > like)?
> > > 
> > Precisely, it makes execbuf ioctl behave like all other ioctls - both
> > vmwgfx and rest of drm.
> 
> But we're still enforcing a non-relaxed size check for the other vmwgfx
> private ioctls, right? Which is relaxed, together with the directions, in
> this commit?
> 
Regardless of the patch, all !execbuf vmwgfx ioctls use the related size
checking from core drm.

> (Not that it matters much to the discussion, though).
> 
Agreed.

> > 
> > > >   - handling of featue flags, as always, is responsibility of the
> > > > driver
> > > > ifself
> > > >   - with this patch, ioctl direction is also handled by core.
> > > > 
> > > > Here core ensures we only copy in/out as much data as the kernel
> > > > implementation can handle.
> > > > 
> > > > 
> > > > Let's consider the following real world example - msm and virtio_gpu.
> > > > 
> > > > An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
> > > >   - we add a flag to annotate/request the out, as always invalid flags
> > > > are return -EINVAL
> > > >   - we change the ioctl encoding
> > > > 
> > > > As currently handled by core DRM, old kernel/new userspace and
> > > > vice-versa works just fine. Sadly, vmwgfx will error out, while it
> > > > could
> > > > be avoided.
> > > IMO basically we have a tradeoff between strict checking in this case,
> > > and new user-space vs old kernel "hazzle-free" transition in the
> > > relaxed case.
> > > 
> > Precisely. If I read the code correctly, ATM new userspace will fail
> > against old kernels. Unless userspace writes two versions of the ioctl -
> > with with each encoding.
> > 
> > > > As said above, I'll gladly adjust core and/or others, if this relaxed
> > > > approach causes an issue somewhere. A specific use-case, real or
> > > > hypothetical will be appreciated.
> > > To me there are two important reasons to keep the strict approach.
> > > 
> > > 1) Avoid user-space mistakes early in the development cycle. We can't
> > > distinguish between buggy user-space and "new" user-space. This is
> > > important because of [a]) below.
> > > 
> > Can you provide a concrete example, please?
> 
> OK, let's say you were developing fence wait functionality. Like
> vmw_fence_obj_wait ioctl. Then suddenly you started to wonder why the wait
> never timed out as it should. The reason turn out to be that signals were
> restarting the waits with the original timeout. So you change the ioctl from
> W to RW and add a kernel-computed time to the argument. Everything is fine,
> except that you forget to change this in a user-space application somewhere.
> 
> So now what happens, is that that user-space bug can live on undetected as
> in 1), and that means you can never go back and implement a stricter check
> because that would completely break old user-space.
> 
If I understand you correctly, the W -> RW change in unnecessary. Yet
the only negative effect that I can see is the copy_to_user() overhead.

The copy should be negligible, yet it "feels" silly.

Is there anything more serious that I've missed?


Having a closer look - vmwgfx (et al) seems to stand out, such that it
does not provide a UABI define including the encoding. Hence it sort of
duplicates that in userspace, by using the explicit drmCommand*

Guess I could follow the suggestion in vmwgfx_drv.c move the defines to
UABI, sync header and update mesa/xf86-video-vmwgfx.

What do you think - yes, or please don't?

> The current code will trap (and has historically trapped) code like this.
> That's mainly why I'm reluctant to give it up, but I guess it can be
> conditionally compiled in for debug purposes.
> 
This piece here, is the holly grail. I'll go further and suggest:

 - add a strict encoding and size check, behind a config toggle
 - make it a core drm thing and drop the custom vmwgfx one

Will keep it disabled by default - but will clearly document Kconfig and
docs that devs should toggle it to catch bugs.

> > 
> > > 2) Catch a lot of fuzzer combinations and error out early instead of
> > > forwarding them to the ioctl function where they may cause harm.
> > > 
> > Struggling to see why this is a problem? At some point the fuzzer will
> > get past this first line of defence, so we want to make the rest of the
> > ioctl is robust.
> > 
> > 
> > > I think the new user-space vs old kernel can be handled nicely in user-
> > > space with feature flags or API versions. That's the way we've handled
> > > them up to now?
> > > 
> > How is a feature flag doing to help if the encoding changes from _IOW
> > to _IORW?
> 
> Ah, you're referring to old user-space new kernel? Yes, I was probably
> reading a bit too fast. Sorry about that.
> 
> So we're basically landing in a tradeoff between trapping problems like
> above, and hazzle-free ioctl argument definition change.
> 
> OK, so I'm ok with that as long as there is a way we can compile in strict
> checking, which will likely has to be as a vmwgfx-specific wrapper.
> 
Ack, I'll proceed with the debug toggle suggestion.

Thank you for the insightful input.
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Thomas Hellstrom]

On 5/27/19 5:27 PM, Emil Velikov wrote:
> On 2019/05/27, Thomas Hellstrom wrote:
>> On 5/27/19 2:35 PM, Emil Velikov wrote:
>>> Hi Thomas,
>>>
>>> On 2019/05/27, Thomas Hellstrom wrote:
>>>
>>>>> I think we might be talking past each other, let's take a step back:
>>>>>
>>>>>    - as of previous patch, all of vmwgfx ioctls size is consistently
>>>>> handled by the core
>>>> I don't think I follow you here, AFAICT patch 3/5 only affects and
>>>> relaxes the execbuf checking (and in fact a little more than I would
>>>> like)?
>>>>
>>> Precisely, it makes execbuf ioctl behave like all other ioctls - both
>>> vmwgfx and rest of drm.
>> But we're still enforcing a non-relaxed size check for the other vmwgfx
>> private ioctls, right? Which is relaxed, together with the directions, in
>> this commit?
>>
> Regardless of the patch, all !execbuf vmwgfx ioctls use the related size
> checking from core drm.

Well it does, but since we (before this patch) enforce ioctl->cmd == 
cmd, we also enforce
_IOC_SIZE(ioctl->cmd) == _IOC_SIZE(cmd), which makes the core check 
pointless, or am I missing something?

>
>> (Not that it matters much to the discussion, though).
>>
> Agreed.
>
>>>
...
>>> Can you provide a concrete example, please?
>> OK, let's say you were developing fence wait functionality. Like
>> vmw_fence_obj_wait ioctl. Then suddenly you started to wonder why the wait
>> never timed out as it should. The reason turn out to be that signals were
>> restarting the waits with the original timeout. So you change the ioctl from
>> W to RW and add a kernel-computed time to the argument. Everything is fine,
>> except that you forget to change this in a user-space application somewhere.
>>
>> So now what happens, is that that user-space bug can live on undetected as
>> in 1), and that means you can never go back and implement a stricter check
>> because that would completely break old user-space.
>>
> If I understand you correctly, the W -> RW change in unnecessary. Yet
> the only negative effect that I can see is the copy_to_user() overhead.
>
> The copy should be negligible, yet it "feels" silly.
>
> Is there anything more serious that I've missed?

Well the point is in this case, that the write was necessary, but the 
code would work sort of OK anyway. It updated a kernel "cookie" to make 
sure the timeout would be correct even with the next call repetition. 
Now if an old header was floating around, there might be clients using 
it. And with the current core checks that typically wouldn't get 
noticed. With the check we'd immediately notice and abort. It feels a 
little like moving from ANSI C to K&R... :-)

>
>
> Having a closer look - vmwgfx (et al) seems to stand out, such that it
> does not provide a UABI define including the encoding. Hence it sort of
> duplicates that in userspace, by using the explicit drmCommand*
>
> Guess I could follow the suggestion in vmwgfx_drv.c move the defines to
> UABI, sync header and update mesa/xf86-video-vmwgfx.
>
> What do you think - yes, or please don't?

Please hold on for a while, and I'll discuss it internally.

>
>> The current code will trap (and has historically trapped) code like this.
>> That's mainly why I'm reluctant to give it up, but I guess it can be
>> conditionally compiled in for debug purposes.
>>
> This piece here, is the holly grail. I'll go further and suggest:
>
>   - add a strict encoding and size check, behind a config toggle
>   - make it a core drm thing and drop the custom vmwgfx one
>
> Will keep it disabled by default - but will clearly document Kconfig and
> docs that devs should toggle it to catch bugs.

Sounds good, but IIRC the reason why I kept it only to driver-private 
ioctls, is that there were errors with the drm ioctls. But that was a 
long time ago so I might remember incorrectly, or user-space has been fixed.

>
>>>> 2) Catch a lot of fuzzer combinations and error out early instead of
>>>> forwarding them to the ioctl function where they may cause harm.
>>>>
>>> Struggling to see why this is a problem? At some point the fuzzer will
>>> get past this first line of defence, so we want to make the rest of the
>>> ioctl is robust.
>>>
>>>
>>>> I think the new user-space vs old kernel can be handled nicely in user-
>>>> space with feature flags or API versions. That's the way we've handled
>>>> them up to now?
>>>>
>>> How is a feature flag doing to help if the encoding changes from _IOW
>>> to _IORW?
>> Ah, you're referring to old user-space new kernel? Yes, I was probably
>> reading a bit too fast. Sorry about that.
>>
>> So we're basically landing in a tradeoff between trapping problems like
>> above, and hazzle-free ioctl argument definition change.
>>
>> OK, so I'm ok with that as long as there is a way we can compile in strict
>> checking, which will likely has to be as a vmwgfx-specific wrapper.
>>
> Ack, I'll proceed with the debug toggle suggestion.

Great.


>
> Thank you for the insightful input.
> Emil

Thanks,

Thomas


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

Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

[Emil Velikov]

On 2019/05/27, Thomas Hellstrom wrote:
> On 5/27/19 5:27 PM, Emil Velikov wrote:
> > On 2019/05/27, Thomas Hellstrom wrote:
> > > On 5/27/19 2:35 PM, Emil Velikov wrote:
> > > > Hi Thomas,
> > > > 
> > > > On 2019/05/27, Thomas Hellstrom wrote:
> > > > 
> > > > > > I think we might be talking past each other, let's take a step back:
> > > > > > 
> > > > > >    - as of previous patch, all of vmwgfx ioctls size is consistently
> > > > > > handled by the core
> > > > > I don't think I follow you here, AFAICT patch 3/5 only affects and
> > > > > relaxes the execbuf checking (and in fact a little more than I would
> > > > > like)?
> > > > > 
> > > > Precisely, it makes execbuf ioctl behave like all other ioctls - both
> > > > vmwgfx and rest of drm.
> > > But we're still enforcing a non-relaxed size check for the other vmwgfx
> > > private ioctls, right? Which is relaxed, together with the directions, in
> > > this commit?
> > > 
> > Regardless of the patch, all !execbuf vmwgfx ioctls use the related size
> > checking from core drm.
> 
> Well it does, but since we (before this patch) enforce ioctl->cmd == cmd, we
> also enforce
> _IOC_SIZE(ioctl->cmd) == _IOC_SIZE(cmd), which makes the core check
> pointless, or am I missing something?
> 
You're spot on - I never looked at the _IOC_SIZE declaration. I was
assuming some other magic.


> > 
> > > (Not that it matters much to the discussion, though).
> > > 
> > Agreed.
> > 
> > > > 
> ...
> > > > Can you provide a concrete example, please?
> > > OK, let's say you were developing fence wait functionality. Like
> > > vmw_fence_obj_wait ioctl. Then suddenly you started to wonder why the wait
> > > never timed out as it should. The reason turn out to be that signals were
> > > restarting the waits with the original timeout. So you change the ioctl from
> > > W to RW and add a kernel-computed time to the argument. Everything is fine,
> > > except that you forget to change this in a user-space application somewhere.
> > > 
> > > So now what happens, is that that user-space bug can live on undetected as
> > > in 1), and that means you can never go back and implement a stricter check
> > > because that would completely break old user-space.
> > > 
> > If I understand you correctly, the W -> RW change in unnecessary. Yet
> > the only negative effect that I can see is the copy_to_user() overhead.
> > 
> > The copy should be negligible, yet it "feels" silly.
> > 
> > Is there anything more serious that I've missed?
> 
> Well the point is in this case, that the write was necessary, but the code
> would work sort of OK anyway. It updated a kernel "cookie" to make sure the
> timeout would be correct even with the next call repetition. Now if an old
> header was floating around, there might be clients using it. And with the
> current core checks that typically wouldn't get noticed. With the check we'd
> immediately notice and abort. It feels a little like moving from ANSI C to
> K&R... :-)
> 
Technically, the kernel (or any function really) should write output
arguments only when needed. Agree though - it's sometimes annoying or
inconvenient.

For the ANSI C to K&R comment - sure, only if one cares about backward
compat. If they don't - flip the config toggle and carry on ;-)

> > 
> > 
> > Having a closer look - vmwgfx (et al) seems to stand out, such that it
> > does not provide a UABI define including the encoding. Hence it sort of
> > duplicates that in userspace, by using the explicit drmCommand*
> > 
> > Guess I could follow the suggestion in vmwgfx_drv.c move the defines to
> > UABI, sync header and update mesa/xf86-video-vmwgfx.
> > 
> > What do you think - yes, or please don't?
> 
> Please hold on for a while, and I'll discuss it internally.
> 
Ack.

> > 
> > > The current code will trap (and has historically trapped) code like this.
> > > That's mainly why I'm reluctant to give it up, but I guess it can be
> > > conditionally compiled in for debug purposes.
> > > 
> > This piece here, is the holly grail. I'll go further and suggest:
> > 
> >   - add a strict encoding and size check, behind a config toggle
> >   - make it a core drm thing and drop the custom vmwgfx one
> > 
> > Will keep it disabled by default - but will clearly document Kconfig and
> > docs that devs should toggle it to catch bugs.
> 
> Sounds good, but IIRC the reason why I kept it only to driver-private
> ioctls, is that there were errors with the drm ioctls. But that was a long
> time ago so I might remember incorrectly, or user-space has been fixed.
> 
Good point - will have a quick look and send patches if needed.

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