drm/vmwgfx: Protect from excessive execbuf kernel memory allocations v2

PATCH In review Thomas Hellstrom 2 participants 4 messages Dec 12, 2018

Subsystems (3)

DRM drivers DRM misc DRM driver for VMware virtual GPU

lore

Share & tools

lore

Thomas Hellstrom

author

2018-12-12 14:36 UTC #0 lore raw

PATCH 1/1 drm/vmwgfx: Protect from excessive execbuf kernel memory allocations v2

With the new validation code, a malicious user-space app could potentially submit command streams with enough buffer-object and resource references in them to have the resulting allocated validion nodes and relocations make the kernel run out of GFP_KERNEL memory.

Protect from this by having the validation code reserve TTM graphics memory when allocating.

Signed-off-by Thomas Hellstrom <[email protected]>

---
v2: Removed leftover debug printouts

---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c        |  4 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h        |  5 +++
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c    |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c   | 36 +++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 18 ++++++++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 37 ++++++++++++++++++++++
 6 files changed, 100 insertions(+), 2 deletions(-)

drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +3 -1

index 61a84b958d67..d7a2dfb8ee9b 100644
@@ -49,6 +49,8 @@
 
 #define VMWGFX_REPO "In Tree"
 
+#define VMWGFX_VALIDATION_MEM_GRAN (16*PAGE_SIZE)
+
 
 /**
  * Fully encoded drm commands. Might move to vmw_drm.h

@@ -918,7 +920,7 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 		spin_unlock(&dev_priv->cap_lock);
 	}
 
-
+	vmw_validation_mem_init_ttm(dev_priv, VMWGFX_VALIDATION_MEM_GRAN);
 	ret = vmw_kms_init(dev_priv);
 	if (unlikely(ret != 0))
 		goto out_no_kms;

drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +5 -0

index 59f614225bcd..aca974b14b55 100644
@@ -606,6 +606,9 @@ struct vmw_private {
 
 	struct vmw_cmdbuf_man *cman;
 	DECLARE_BITMAP(irqthread_pending, VMW_IRQTHREAD_MAX);
+
+	/* Validation memory reservation */
+	struct vmw_validation_mem vvm;
 };
 
 static inline struct vmw_surface *vmw_res_to_srf(struct vmw_resource *res)

@@ -846,6 +849,8 @@ extern int vmw_ttm_global_init(struct vmw_private *dev_priv);
 extern void vmw_ttm_global_release(struct vmw_private *dev_priv);
 extern int vmw_mmap(struct file *filp, struct vm_area_struct *vma);
 
+extern void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv,
+					size_t gran);
 /**
  * TTM buffer object driver - vmwgfx_ttm_buffer.c
  */

drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +2 -0

index 260650bb5560..f2d13a72c05d 100644
@@ -3835,6 +3835,8 @@ int vmw_execbuf_process(struct drm_file *file_priv,
 	struct sync_file *sync_file = NULL;
 	DECLARE_VAL_CONTEXT(val_ctx, &sw_context->res_ht, 1);
 
+	vmw_validation_set_val_mem(&val_ctx, &dev_priv->vvm);
+
 	if (flags & DRM_VMW_EXECBUF_FLAG_EXPORT_FENCE_FD) {
 		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
 		if (out_fence_fd < 0) {

drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +36 -0

index 7b1e5a5cbd2c..f88247046721 100644
@@ -96,3 +96,39 @@ void vmw_ttm_global_release(struct vmw_private *dev_priv)
 	drm_global_item_unref(&dev_priv->bo_global_ref.ref);
 	drm_global_item_unref(&dev_priv->mem_global_ref);
 }
+
+/* struct vmw_validation_mem callback */
+static int vmw_vmt_reserve(struct vmw_validation_mem *m, size_t size)
+{
+	static struct ttm_operation_ctx ctx = {.interruptible = false,
+					       .no_wait_gpu = false};
+	struct vmw_private *dev_priv = container_of(m, struct vmw_private, vvm);
+
+	return ttm_mem_global_alloc(vmw_mem_glob(dev_priv), size, &ctx);
+}
+
+/* struct vmw_validation_mem callback */
+static void vmw_vmt_unreserve(struct vmw_validation_mem *m, size_t size)
+{
+	struct vmw_private *dev_priv = container_of(m, struct vmw_private, vvm);
+
+	return ttm_mem_global_free(vmw_mem_glob(dev_priv), size);
+}
+
+/**
+ * vmw_validation_mem_init_ttm - Interface the validation memory tracker
+ * to ttm.
+ * @dev_priv: Pointer to struct vmw_private. The reason we choose a vmw private
+ * rather than a struct vmw_validation_mem is to make sure assumption in the
+ * callbacks that struct vmw_private derives from struct vmw_validation_mem
+ * holds true.
+ * @gran: The recommended allocation granularity
+ */
+void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv, size_t gran)
+{
+	struct vmw_validation_mem *vvm = &dev_priv->vvm;
+
+	vvm->reserve_mem = vmw_vmt_reserve;
+	vvm->unreserve_mem = vmw_vmt_unreserve;
+	vvm->gran = gran;
+}

drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +17 -1

index 184025fa938e..1f83b90fd259 100644
@@ -104,11 +104,25 @@ void *vmw_validation_mem_alloc(struct vmw_validation_context *ctx,
 		return NULL;
 
 	if (ctx->mem_size_left < size) {
-		struct page *page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+		struct page *page;
 
+		if (ctx->vm && ctx->vm_size_left < PAGE_SIZE) {
+			int ret = ctx->vm->reserve_mem(ctx->vm, ctx->vm->gran);
+
+			if (ret)
+				return NULL;
+
+			ctx->vm_size_left += ctx->vm->gran;
+			ctx->total_mem += ctx->vm->gran;
+		}
+
+		page = alloc_page(GFP_KERNEL | __GFP_ZERO);
 		if (!page)
 			return NULL;
 
+		if (ctx->vm)
+			ctx->vm_size_left -= PAGE_SIZE;
+
 		list_add_tail(&page->lru, &ctx->page_list);
 		ctx->page_address = page_address(page);
 		ctx->mem_size_left = PAGE_SIZE;

@@ -138,6 +152,8 @@ static void vmw_validation_mem_free(struct vmw_validation_context *ctx)
 	}
 
 	ctx->mem_size_left = 0;
+	if (ctx->vm && ctx->total_mem)
+		ctx->vm->unreserve_mem(ctx->vm, ctx->total_mem);
 }
 
 /**

drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +37 -0

index b57e3292c386..3b396fea40d7 100644
@@ -33,6 +33,21 @@
 #include <linux/ww_mutex.h>
 #include <drm/ttm/ttm_execbuf_util.h>
 
+/**
+ * struct vmw_validation_mem - Custom interface to provide memory reservations
+ * for the validation code.
+ * @reserve_mem: Callback to reserve memory
+ * @unreserve_mem: Callback to unreserve memory
+ * @gran: Reservation granularity. Contains a hint how much memory should
+ * be reserved in each call to @reserve_mem(). A slow implementation may want
+ * reservation to be done in large batches.
+ */
+struct vmw_validation_mem {
+	int (*reserve_mem)(struct vmw_validation_mem *m, size_t size);
+	void (*unreserve_mem)(struct vmw_validation_mem *m, size_t size);
+	size_t gran;
+};
+
 /**
  * struct vmw_validation_context - Per command submission validation context
  * @ht: Hash table used to find resource- or buffer object duplicates

@@ -47,6 +62,10 @@
  * buffer objects
  * @mem_size_left: Free memory left in the last page in @page_list
  * @page_address: Kernel virtual address of the last page in @page_list
+ * @vm: A pointer to the memory reservation interface or NULL if no
+ * memory reservation is needed.
+ * @vm_size_left: Amount of reserved memory that so far has not been allocated.
+ * @total_mem: Amount of reserved memory.
  */
 struct vmw_validation_context {
 	struct drm_open_hash *ht;

@@ -59,6 +78,9 @@ struct vmw_validation_context {
 	unsigned int merge_dups;
 	unsigned int mem_size_left;
 	u8 *page_address;
+	struct vmw_validation_mem *vm;
+	size_t vm_size_left;
+	size_t total_mem;
 };
 
 struct vmw_buffer_object;

@@ -101,6 +123,21 @@ vmw_validation_has_bos(struct vmw_validation_context *ctx)
 	return !list_empty(&ctx->bo_list);
 }
 
+/**
+ * vmw_validation_set_val_mem - Register a validation mem object for
+ * validation memory reservation
+ * @ctx: The validation context
+ * @vm: Pointer to a struct vmw_validation_mem
+ *
+ * Must be set before the first attempt to allocate validation memory.
+ */
+static inline void
+vmw_validation_set_val_mem(struct vmw_validation_context *ctx,
+			   struct vmw_validation_mem *vm)
+{
+	ctx->vm = vm;
+}
+
 /**
  * vmw_validation_set_ht - Register a hash table for duplicate finding
  * @ctx: The validation context

Signature · 6 lines

2.19.0.rc1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Deepak Singh Rawat

2018-12-12 18:00 UTC #1 lore raw

Quoted 152 lines · Thomas Hellstrom

With the new validation code, a malicious user-space app could potentially submit command streams with enough buffer-object and resource
references in them to have the resulting allocated validion nodes and relocations make the kernel run out of GFP_KERNEL memory.
Protect from this by having the validation code reserve TTM graphics memory when allocating.
Signed-off-by Thomas Hellstrom <[email protected]>
---
v2: Removed leftover debug printouts
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c        |  4 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h        |  5 +++
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c    |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c   | 36
+++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 18 ++++++++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 37
++++++++++++++++++++++
 6 files changed, 100 insertions(+), 2 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +3 -1
index 61a84b958d67..d7a2dfb8ee9b 100644
@@ -49,6 +49,8 @@
 
 #define VMWGFX_REPO "In Tree"
 
+#define VMWGFX_VALIDATION_MEM_GRAN (16*PAGE_SIZE)
+
 
 /**
  * Fully encoded drm commands. Might move to vmw_drm.h
@@ -918,7 +920,7 @@ static int vmw_driver_load(struct drm_device
 *dev, unsigned long chipset)
 		spin_unlock(&dev_priv->cap_lock);
 	}
 
-
+	vmw_validation_mem_init_ttm(dev_priv,
 VMWGFX_VALIDATION_MEM_GRAN);
 	ret = vmw_kms_init(dev_priv);
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +0 -0
 	if (unlikely(ret != 0))
 		goto out_no_kms;
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +3 -0
index 59f614225bcd..aca974b14b55 100644
@@ -606,6 +606,9 @@ struct vmw_private {
 
 	struct vmw_cmdbuf_man *cman;
 	DECLARE_BITMAP(irqthread_pending, VMW_IRQTHREAD_MAX);
+
+	/* Validation memory reservation */
+	struct vmw_validation_mem vvm;
 };
 
 static inline struct vmw_surface *vmw_res_to_srf(struct vmw_resource
*res)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +2 -0
@@ -846,6 +849,8 @@ extern int vmw_ttm_global_init(struct vmw_private
 *dev_priv);
 extern void vmw_ttm_global_release(struct vmw_private *dev_priv);
 extern int vmw_mmap(struct file *filp, struct vm_area_struct *vma);
 
+extern void vmw_validation_mem_init_ttm(struct vmw_private
 *dev_priv,
+					size_t gran);
 /**
* TTM buffer object driver - vmwgfx_ttm_buffer.c */
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +2 -0
index 260650bb5560..f2d13a72c05d 100644
@@ -3835,6 +3835,8 @@ int vmw_execbuf_process(struct drm_file
 *file_priv,
 	struct sync_file *sync_file = NULL;
 	DECLARE_VAL_CONTEXT(val_ctx, &sw_context->res_ht, 1);
 
+	vmw_validation_set_val_mem(&val_ctx, &dev_priv->vvm);
+
 	if (flags & DRM_VMW_EXECBUF_FLAG_EXPORT_FENCE_FD) {
 		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +0 -0
 		if (out_fence_fd < 0) {
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +3 -0
index 7b1e5a5cbd2c..f88247046721 100644
@@ -96,3 +96,39 @@ void vmw_ttm_global_release(struct vmw_private
 *dev_priv)
 	drm_global_item_unref(&dev_priv->bo_global_ref.ref);
 	drm_global_item_unref(&dev_priv->mem_global_ref);
 }
+
+/* struct vmw_validation_mem callback */
+static int vmw_vmt_reserve(struct vmw_validation_mem *m, size_t
size)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +4 -0
+{
+	static struct ttm_operation_ctx ctx = {.interruptible = false,
+					       .no_wait_gpu = false};
+	struct vmw_private *dev_priv = container_of(m, struct
vmw_private, vvm);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +2 -0
+
+	return ttm_mem_global_alloc(vmw_mem_glob(dev_priv), size,
&ctx);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +4 -0
+}
+
+/* struct vmw_validation_mem callback */
+static void vmw_vmt_unreserve(struct vmw_validation_mem *m, size_t
size)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +2 -0
+{
+	struct vmw_private *dev_priv = container_of(m, struct
vmw_private, vvm);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +6 -0
+
+	return ttm_mem_global_free(vmw_mem_glob(dev_priv), size);
+}
+
+/**
+ * vmw_validation_mem_init_ttm - Interface the validation memory
tracker
+ * to ttm.
+ * @dev_priv: Pointer to struct vmw_private. The reason we choose a vmw private
+ * rather than a struct vmw_validation_mem is to make sure assumption in the
+ * callbacks that struct vmw_private derives from struct vmw_validation_mem
+ * holds true.
+ * @gran: The recommended allocation granularity
+ */
+void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv, size_t gran)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +7 -0
+{
+	struct vmw_validation_mem *vvm = &dev_priv->vvm;
+
+	vvm->reserve_mem = vmw_vmt_reserve;
+	vvm->unreserve_mem = vmw_vmt_unreserve;
+	vvm->gran = gran;
+}
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +0 -0
index 184025fa938e..1f83b90fd259 100644

drivers/gpu/drm/vmwgfx/vmwgfx_validation.c void *vmw_validation_mem_alloc(struct · line 111

Quoted 9 lines

drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +2 -1

@@ -104,11 +104,25 @@ void *vmw_validation_mem_alloc(struct
 vmw_validation_context *ctx,
 		return NULL;
 
 	if (ctx->mem_size_left < size) {
-		struct page *page = alloc_page(GFP_KERNEL |
 __GFP_ZERO);
+		struct page *page;
 
+		if (ctx->vm && ctx->vm_size_left < PAGE_SIZE) {

I quite didn't understand the logic here but ctx->vm_size_left is set to GRAN size which is 16 * PAGE_SIZE so it will go inside this condition for one time only. Is that what is intended here? Also I guess that is why not resetting ctx->vm_size_left ?

Other than that looks good to me.

Quoted 109 lines

drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +13 -0

+			int ret = ctx->vm->reserve_mem(ctx->vm, ctx-
 >vm->gran);
+
+			if (ret)
+				return NULL;
+
+			ctx->vm_size_left += ctx->vm->gran;
+			ctx->total_mem += ctx->vm->gran;
+		}
+
+		page = alloc_page(GFP_KERNEL | __GFP_ZERO);
 		if (!page)
 			return NULL;
 
+		if (ctx->vm)
+			ctx->vm_size_left -= PAGE_SIZE;
+

drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +2 -0

 		list_add_tail(&page->lru, &ctx->page_list);
 		ctx->page_address = page_address(page);
 		ctx->mem_size_left = PAGE_SIZE;

@@ -138,6 +152,8 @@ static void vmw_validation_mem_free(struct
 vmw_validation_context *ctx)
 	}
 
 	ctx->mem_size_left = 0;
+	if (ctx->vm && ctx->total_mem)
+		ctx->vm->unreserve_mem(ctx->vm, ctx->total_mem);
 }

/**

drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +2 -0

index b57e3292c386..3b396fea40d7 100644
@@ -33,6 +33,21 @@
 #include <linux/ww_mutex.h>
 #include <drm/ttm/ttm_execbuf_util.h>
 
+/**
+ * struct vmw_validation_mem - Custom interface to provide memory

reservations
+ * for the validation code.
+ * @reserve_mem: Callback to reserve memory
+ * @unreserve_mem: Callback to unreserve memory
+ * @gran: Reservation granularity. Contains a hint how much memory should
+ * be reserved in each call to @reserve_mem(). A slow implementation may want
+ * reservation to be done in large batches.
+ */

drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0

+struct vmw_validation_mem {
+	int (*reserve_mem)(struct vmw_validation_mem *m, size_t size);
+	void (*unreserve_mem)(struct vmw_validation_mem *m, size_t

size);

drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0

+	size_t gran;
+};
+
 /**
  * struct vmw_validation_context - Per command submission validation

context
* @ht: Hash table used to find resource- or buffer object
duplicates

drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +0 -0

@@ -47,6 +62,10 @@
  * buffer objects
  * @mem_size_left: Free memory left in the last page in @page_list
  * @page_address: Kernel virtual address of the last page in

@page_list
+ * @vm: A pointer to the memory reservation interface or NULL if no
+ * memory reservation is needed.
+ * @vm_size_left: Amount of reserved memory that so far has not been allocated.
+ * @total_mem: Amount of reserved memory.
*/
struct vmw_validation_context {

drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +10 -0

 	struct drm_open_hash *ht;

@@ -59,6 +78,9 @@ struct vmw_validation_context {
 	unsigned int merge_dups;
 	unsigned int mem_size_left;
 	u8 *page_address;
+	struct vmw_validation_mem *vm;
+	size_t vm_size_left;
+	size_t total_mem;
 };
 
 struct vmw_buffer_object;

@@ -101,6 +123,21 @@ vmw_validation_has_bos(struct
 vmw_validation_context *ctx)
 	return !list_empty(&ctx->bo_list);
 }
 
+/**
+ * vmw_validation_set_val_mem - Register a validation mem object for
+ * validation memory reservation
+ * @ctx: The validation context
+ * @vm: Pointer to a struct vmw_validation_mem
+ *
+ * Must be set before the first attempt to allocate validation

memory.
+ */

drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +7 -0

+static inline void
+vmw_validation_set_val_mem(struct vmw_validation_context *ctx,
+			   struct vmw_validation_mem *vm)
+{
+	ctx->vm = vm;
+}
+
 /**
  * vmw_validation_set_ht - Register a hash table for duplicate

Quoted 2 lines

finding
* @ctx: The validation context

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thomas Hellstrom

author

2018-12-12 18:38 UTC #2 lore raw

Hi, Deepak,

Quoted 167 lines · Deepak Singh Rawat

Quoted 166 lines · Thomas Hellstrom
With the new validation code, a malicious user-space app could potentially submit command streams with enough buffer-object and resource
references in them to have the resulting allocated validion nodes and
relocations make the kernel run out of GFP_KERNEL memory.
Protect from this by having the validation code reserve TTM graphics
memory when allocating.
Signed-off-by Thomas Hellstrom <[email protected]>
---
v2: Removed leftover debug printouts
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c        |  4 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h        |  5 +++
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c    |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c   | 36
+++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 18 ++++++++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 37
++++++++++++++++++++++
 6 files changed, 100 insertions(+), 2 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +3 -1
index 61a84b958d67..d7a2dfb8ee9b 100644
@@ -49,6 +49,8 @@
 
 #define VMWGFX_REPO "In Tree"
 
+#define VMWGFX_VALIDATION_MEM_GRAN (16*PAGE_SIZE)
+
 
 /**
  * Fully encoded drm commands. Might move to vmw_drm.h
@@ -918,7 +920,7 @@ static int vmw_driver_load(struct drm_device
 *dev, unsigned long chipset)
 		spin_unlock(&dev_priv->cap_lock);
 	}
 
-
+	vmw_validation_mem_init_ttm(dev_priv,
 VMWGFX_VALIDATION_MEM_GRAN);
 	ret = vmw_kms_init(dev_priv);
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +0 -0
 	if (unlikely(ret != 0))
 		goto out_no_kms;
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +3 -0
index 59f614225bcd..aca974b14b55 100644
@@ -606,6 +606,9 @@ struct vmw_private {
 
 	struct vmw_cmdbuf_man *cman;
 	DECLARE_BITMAP(irqthread_pending, VMW_IRQTHREAD_MAX);
+
+	/* Validation memory reservation */
+	struct vmw_validation_mem vvm;
 };
 
 static inline struct vmw_surface *vmw_res_to_srf(struct
vmw_resource
*res)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +0 -0
@@ -846,6 +849,8 @@ extern int vmw_ttm_global_init(struct
vmw_private
*dev_priv);
extern void vmw_ttm_global_release(struct vmw_private *dev_priv); extern int vmw_mmap(struct file *filp, struct vm_area_struct
*vma);
+extern void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv,
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +1 -0
+					size_t gran);
 /**
  * TTM buffer object driver - vmwgfx_ttm_buffer.c
  */
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +2 -0
index 260650bb5560..f2d13a72c05d 100644
@@ -3835,6 +3835,8 @@ int vmw_execbuf_process(struct drm_file
 *file_priv,
 	struct sync_file *sync_file = NULL;
 	DECLARE_VAL_CONTEXT(val_ctx, &sw_context->res_ht, 1);
 
+	vmw_validation_set_val_mem(&val_ctx, &dev_priv->vvm);
+
 	if (flags & DRM_VMW_EXECBUF_FLAG_EXPORT_FENCE_FD) {
 		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +0 -0
 		if (out_fence_fd < 0) {
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +3 -0
index 7b1e5a5cbd2c..f88247046721 100644
@@ -96,3 +96,39 @@ void vmw_ttm_global_release(struct vmw_private
 *dev_priv)
 	drm_global_item_unref(&dev_priv->bo_global_ref.ref);
 	drm_global_item_unref(&dev_priv->mem_global_ref);
 }
+
+/* struct vmw_validation_mem callback */
+static int vmw_vmt_reserve(struct vmw_validation_mem *m, size_t
size)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +4 -0
+{
+	static struct ttm_operation_ctx ctx = {.interruptible = false,
+					       .no_wait_gpu = false};
+	struct vmw_private *dev_priv = container_of(m, struct
vmw_private, vvm);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +2 -0
+
+	return ttm_mem_global_alloc(vmw_mem_glob(dev_priv), size,
&ctx);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +4 -0
+}
+
+/* struct vmw_validation_mem callback */
+static void vmw_vmt_unreserve(struct vmw_validation_mem *m, size_t
size)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +2 -0
+{
+	struct vmw_private *dev_priv = container_of(m, struct
vmw_private, vvm);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +6 -0
+
+	return ttm_mem_global_free(vmw_mem_glob(dev_priv), size);
+}
+
+/**
+ * vmw_validation_mem_init_ttm - Interface the validation memory
tracker
+ * to ttm.
+ * @dev_priv: Pointer to struct vmw_private. The reason we choose a
vmw private
+ * rather than a struct vmw_validation_mem is to make sure assumption in the
+ * callbacks that struct vmw_private derives from struct vmw_validation_mem
+ * holds true.
+ * @gran: The recommended allocation granularity
+ */
+void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv, size_t gran)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +7 -0
+{
+	struct vmw_validation_mem *vvm = &dev_priv->vvm;
+
+	vvm->reserve_mem = vmw_vmt_reserve;
+	vvm->unreserve_mem = vmw_vmt_unreserve;
+	vvm->gran = gran;
+}
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +1 -1
index 184025fa938e..1f83b90fd259 100644
@@ -104,11 +104,25 @@ void *vmw_validation_mem_alloc(struct
 vmw_validation_context *ctx,
 		return NULL;
 
 	if (ctx->mem_size_left < size) {
-		struct page *page = alloc_page(GFP_KERNEL |
 __GFP_ZERO);
+		struct page *page;
 

Quoted 6 lines

drivers/gpu/drm/vmwgfx/vmwgfx_validation.c void *vmw_validation_mem_alloc(struct vmw_validation_context *ctx, · line 109
Quoted 1 line
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +1 -0
+		if (ctx->vm && ctx->vm_size_left < PAGE_SIZE) {
I quite didn't understand the logic here but ctx->vm_size_left is set to GRAN size which is 16 * PAGE_SIZE so it will go inside this condition for one time only. Is that what is intended here? Also I guess that is why not resetting ctx->vm_size_left ?

ctx->vm_size_left is decreased with PAGE_SIZE after allocating a page, which means we do a new reservation each 16 pages, but typically that much memory is very seldom used, so in essence we will do a memory reservation once per command buffer. Although it's correct we should clear ctx->vm_size_left when we unreserve.

/Thomas

Quoted 111 lines

Other than that looks good to me.
Quoted 108 lines
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +13 -0
+			int ret = ctx->vm->reserve_mem(ctx->vm, ctx-
 > vm->gran);
+
+			if (ret)
+				return NULL;
+
+			ctx->vm_size_left += ctx->vm->gran;
+			ctx->total_mem += ctx->vm->gran;
+		}
+
+		page = alloc_page(GFP_KERNEL | __GFP_ZERO);
 		if (!page)
 			return NULL;
 
+		if (ctx->vm)
+			ctx->vm_size_left -= PAGE_SIZE;
+
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +2 -0
 		list_add_tail(&page->lru, &ctx->page_list);
 		ctx->page_address = page_address(page);
 		ctx->mem_size_left = PAGE_SIZE;
@@ -138,6 +152,8 @@ static void vmw_validation_mem_free(struct
 vmw_validation_context *ctx)
 	}
 
 	ctx->mem_size_left = 0;
+	if (ctx->vm && ctx->total_mem)
+		ctx->vm->unreserve_mem(ctx->vm, ctx->total_mem);
 }
 
/**
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +2 -0
index b57e3292c386..3b396fea40d7 100644
@@ -33,6 +33,21 @@
 #include <linux/ww_mutex.h>
 #include <drm/ttm/ttm_execbuf_util.h>
 
+/**
+ * struct vmw_validation_mem - Custom interface to provide memory
reservations
+ * for the validation code.
+ * @reserve_mem: Callback to reserve memory
+ * @unreserve_mem: Callback to unreserve memory
+ * @gran: Reservation granularity. Contains a hint how much memory should
+ * be reserved in each call to @reserve_mem(). A slow implementation
may want
+ * reservation to be done in large batches.
+ */
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0
+struct vmw_validation_mem {
+	int (*reserve_mem)(struct vmw_validation_mem *m, size_t size);
+	void (*unreserve_mem)(struct vmw_validation_mem *m, size_t
size);
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0
+	size_t gran;
+};
+
 /**
  * struct vmw_validation_context - Per command submission
validation
context
* @ht: Hash table used to find resource- or buffer object
duplicates
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +0 -0
@@ -47,6 +62,10 @@
  * buffer objects
  * @mem_size_left: Free memory left in the last page in @page_list
  * @page_address: Kernel virtual address of the last page in
@page_list
+ * @vm: A pointer to the memory reservation interface or NULL if no
+ * memory reservation is needed.
+ * @vm_size_left: Amount of reserved memory that so far has not been
allocated.
+ * @total_mem: Amount of reserved memory.
*/
struct vmw_validation_context {
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +5 -0
 	struct drm_open_hash *ht;
@@ -59,6 +78,9 @@ struct vmw_validation_context {
 	unsigned int merge_dups;
 	unsigned int mem_size_left;
 	u8 *page_address;
+	struct vmw_validation_mem *vm;
+	size_t vm_size_left;
+	size_t total_mem;
 };
 
 struct vmw_buffer_object;
@@ -101,6 +123,21 @@ vmw_validation_has_bos(struct
 vmw_validation_context *ctx)
 	return !list_empty(&ctx->bo_list);
 }
 
+/**
+ * vmw_validation_set_val_mem - Register a validation mem object
for
+ * validation memory reservation
+ * @ctx: The validation context
+ * @vm: Pointer to a struct vmw_validation_mem
+ *
+ * Must be set before the first attempt to allocate validation memory.
+ */
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0
+static inline void
+vmw_validation_set_val_mem(struct vmw_validation_context *ctx,
+			   struct vmw_validation_mem *vm)

Quoted 8 lines

Quoted 8 lines
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +4 -0
+{
+	ctx->vm = vm;
+}
+
 /**
  * vmw_validation_set_ht - Register a hash table for duplicate
finding
* @ctx: The validation context

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Deepak Singh Rawat

Reviewed-by

2018-12-12 18:43 UTC #3 lore raw

Reviewed-by Deepak Rawat <[email protected]>

Quoted 182 lines · Thomas Hellstrom

Hi, Deepak,
Quoted 170 lines · Deepak Singh Rawat
Quoted 169 lines · Thomas Hellstrom
With the new validation code, a malicious user-space app could potentially submit command streams with enough buffer-object and resource
references in them to have the resulting allocated validion nodes and
relocations make the kernel run out of GFP_KERNEL memory.
Protect from this by having the validation code reserve TTM graphics
memory when allocating.
Signed-off-by Thomas Hellstrom <[email protected]>
---
v2: Removed leftover debug printouts
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c        |  4 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h        |  5 +++
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c    |  2 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c   | 36
+++++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 18 ++++++++++-
 drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 37
++++++++++++++++++++++
 6 files changed, 100 insertions(+), 2 deletions(-)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +3 -1
index 61a84b958d67..d7a2dfb8ee9b 100644
@@ -49,6 +49,8 @@
 
 #define VMWGFX_REPO "In Tree"
 
+#define VMWGFX_VALIDATION_MEM_GRAN (16*PAGE_SIZE)
+
 
 /**
  * Fully encoded drm commands. Might move to vmw_drm.h
@@ -918,7 +920,7 @@ static int vmw_driver_load(struct drm_device
 *dev, unsigned long chipset)
 		spin_unlock(&dev_priv->cap_lock);
 	}
 
-
+	vmw_validation_mem_init_ttm(dev_priv,
 VMWGFX_VALIDATION_MEM_GRAN);
 	ret = vmw_kms_init(dev_priv);
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +0 -0
 	if (unlikely(ret != 0))
 		goto out_no_kms;
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +3 -0
index 59f614225bcd..aca974b14b55 100644
@@ -606,6 +606,9 @@ struct vmw_private {
 
 	struct vmw_cmdbuf_man *cman;
 	DECLARE_BITMAP(irqthread_pending, VMW_IRQTHREAD_MAX);
+
+	/* Validation memory reservation */
+	struct vmw_validation_mem vvm;
 };
 
 static inline struct vmw_surface *vmw_res_to_srf(struct
vmw_resource
*res)
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +0 -0
@@ -846,6 +849,8 @@ extern int vmw_ttm_global_init(struct
vmw_private
*dev_priv);
extern void vmw_ttm_global_release(struct vmw_private
*dev_priv);
extern int vmw_mmap(struct file *filp, struct vm_area_struct
*vma);
+extern void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv,
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +1 -0
+					size_t gran);
 /**
  * TTM buffer object driver - vmwgfx_ttm_buffer.c
  */
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +2 -0
index 260650bb5560..f2d13a72c05d 100644
@@ -3835,6 +3835,8 @@ int vmw_execbuf_process(struct drm_file
 *file_priv,
 	struct sync_file *sync_file = NULL;
 	DECLARE_VAL_CONTEXT(val_ctx, &sw_context->res_ht, 1);
 
+	vmw_validation_set_val_mem(&val_ctx, &dev_priv->vvm);
+
 	if (flags & DRM_VMW_EXECBUF_FLAG_EXPORT_FENCE_FD) {
 		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +0 -0
 		if (out_fence_fd < 0) {
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +3 -0
index 7b1e5a5cbd2c..f88247046721 100644
@@ -96,3 +96,39 @@ void vmw_ttm_global_release(struct vmw_private
 *dev_priv)
 	drm_global_item_unref(&dev_priv->bo_global_ref.ref);
 	drm_global_item_unref(&dev_priv->mem_global_ref);
 }
+
+/* struct vmw_validation_mem callback */
+static int vmw_vmt_reserve(struct vmw_validation_mem *m, size_t
size)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +4 -0
+{
+	static struct ttm_operation_ctx ctx = {.interruptible = false,
+					       .no_wait_gpu = false};
+	struct vmw_private *dev_priv = container_of(m, struct
vmw_private, vvm);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +2 -0
+
+	return ttm_mem_global_alloc(vmw_mem_glob(dev_priv), size,
&ctx);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +4 -0
+}
+
+/* struct vmw_validation_mem callback */
+static void vmw_vmt_unreserve(struct vmw_validation_mem *m,
size_t
size)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +2 -0
+{
+	struct vmw_private *dev_priv = container_of(m, struct
vmw_private, vvm);
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +6 -0
+
+	return ttm_mem_global_free(vmw_mem_glob(dev_priv), size);
+}
+
+/**
+ * vmw_validation_mem_init_ttm - Interface the validation memory
tracker
+ * to ttm.
+ * @dev_priv: Pointer to struct vmw_private. The reason we choose
a
vmw private
+ * rather than a struct vmw_validation_mem is to make sure assumption in the
+ * callbacks that struct vmw_private derives from struct vmw_validation_mem
+ * holds true.
+ * @gran: The recommended allocation granularity
+ */
+void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv, size_t gran)
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +7 -0
+{
+	struct vmw_validation_mem *vvm = &dev_priv->vvm;
+
+	vvm->reserve_mem = vmw_vmt_reserve;
+	vvm->unreserve_mem = vmw_vmt_unreserve;
+	vvm->gran = gran;
+}
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +1 -1
index 184025fa938e..1f83b90fd259 100644
@@ -104,11 +104,25 @@ void *vmw_validation_mem_alloc(struct
 vmw_validation_context *ctx,
 		return NULL;
 
 	if (ctx->mem_size_left < size) {
-		struct page *page = alloc_page(GFP_KERNEL |
 __GFP_ZERO);
+		struct page *page;
 
Quoted 7 lines
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c void *vmw_validation_mem_alloc(struct vmw_validation_context *ctx, · line 109
Quoted 1 line
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +1 -0
+		if (ctx->vm && ctx->vm_size_left < PAGE_SIZE) {
I quite didn't understand the logic here but ctx->vm_size_left is set
to GRAN size which is 16 * PAGE_SIZE so it will go inside this condition for one time only. Is that what is intended here? Also I guess that is why not resetting ctx->vm_size_left ?

Quoted 8 lines

ctx->vm_size_left is decreased with PAGE_SIZE after allocating a page,
which means we do a new reservation each 16 pages, but typically that much memory is very seldom used, so in essence we will do a memory reservation once per command buffer. Although it's correct we should clear ctx->vm_size_left when we unreserve.
/Thomas

Yes, got it. Overlooked the decrease as error handling and didn't realize the free function is to free all memory.

Quoted 117 lines

Quoted 117 lines
Other than that looks good to me.
Quoted 112 lines
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +15 -0
+			int ret = ctx->vm->reserve_mem(ctx->vm, ctx-
 > vm->gran);
 
+
+			if (ret)
+				return NULL;
+
+			ctx->vm_size_left += ctx->vm->gran;
+			ctx->total_mem += ctx->vm->gran;
+		}
+
+		page = alloc_page(GFP_KERNEL | __GFP_ZERO);
 		if (!page)
 			return NULL;
 
+		if (ctx->vm)
+			ctx->vm_size_left -= PAGE_SIZE;
+
 		list_add_tail(&page->lru, &ctx->page_list);
 		ctx->page_address = page_address(page);
 		ctx->mem_size_left = PAGE_SIZE;
@@ -138,6 +152,8 @@ static void vmw_validation_mem_free(struct
 vmw_validation_context *ctx)
 	}
 
 	ctx->mem_size_left = 0;
+	if (ctx->vm && ctx->total_mem)
+		ctx->vm->unreserve_mem(ctx->vm, ctx->total_mem);
 }
 
/**
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +2 -0
index b57e3292c386..3b396fea40d7 100644
@@ -33,6 +33,21 @@
 #include <linux/ww_mutex.h>
 #include <drm/ttm/ttm_execbuf_util.h>
 
+/**
+ * struct vmw_validation_mem - Custom interface to provide
memory
reservations
+ * for the validation code.
+ * @reserve_mem: Callback to reserve memory
+ * @unreserve_mem: Callback to unreserve memory
+ * @gran: Reservation granularity. Contains a hint how much memory
should
+ * be reserved in each call to @reserve_mem(). A slow implementation
may want
+ * reservation to be done in large batches.
+ */
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0
+struct vmw_validation_mem {
+	int (*reserve_mem)(struct vmw_validation_mem *m, size_t size);
+	void (*unreserve_mem)(struct vmw_validation_mem *m, size_t
size);
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0
+	size_t gran;
+};
+
 /**
  * struct vmw_validation_context - Per command submission
validation
context
* @ht: Hash table used to find resource- or buffer object
duplicates
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +0 -0
@@ -47,6 +62,10 @@
  * buffer objects
  * @mem_size_left: Free memory left in the last page in
@page_list
* @page_address: Kernel virtual address of the last page in
@page_list
+ * @vm: A pointer to the memory reservation interface or NULL if no
+ * memory reservation is needed.
+ * @vm_size_left: Amount of reserved memory that so far has not been
allocated.
+ * @total_mem: Amount of reserved memory.
*/
struct vmw_validation_context {
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +5 -0
 	struct drm_open_hash *ht;
@@ -59,6 +78,9 @@ struct vmw_validation_context {
 	unsigned int merge_dups;
 	unsigned int mem_size_left;
 	u8 *page_address;
+	struct vmw_validation_mem *vm;
+	size_t vm_size_left;
+	size_t total_mem;
 };
 
 struct vmw_buffer_object;
@@ -101,6 +123,21 @@ vmw_validation_has_bos(struct
 vmw_validation_context *ctx)
 	return !list_empty(&ctx->bo_list);
 }
 
+/**
+ * vmw_validation_set_val_mem - Register a validation mem object
for
+ * validation memory reservation
+ * @ctx: The validation context
+ * @vm: Pointer to a struct vmw_validation_mem
+ *
+ * Must be set before the first attempt to allocate validation memory.
+ */
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +3 -0
+static inline void
+vmw_validation_set_val_mem(struct vmw_validation_context *ctx,
+			   struct vmw_validation_mem *vm)

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h +4 -0
+{
+	ctx->vm = vm;
+}
+
 /**
  * vmw_validation_set_ht - Register a hash table for duplicate
finding
* @ctx: The validation context

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel