[PATCH 0/5] HW_LOCK Security Patches

[PATCH 0/5] HW_LOCK Security Patches

[Peter Antoine]

The following series of patches fix a number of security holes in the i915
driver (actually in drm but...). The first three patches remove the actual
security issues that have been found. The last two patches make the functions
optional for all drivers. The only driver that has this feature turned on is
the Nouveau driver, thus hopefully not breaking that driver.

The patch set has been tested on the Intel platforms but has not been tested
on the Nouveau driver. Hopefully someone with a working card and the right
combination of drmlib can verify that the patches do what they say.

There is a i-g-t test that goes with this patchset, but that test SHOULD NOT
be run before the kernel is patches as the test will crash the driver and/or
make the kernel panic.

Peter.

Peter Antoine (5):
  drm: Kernel Crash in drm_unlock
  drm: Fixes unsafe deference in locks.
  drm: Possible lock priority escalation.
  drm: Make HW_LOCK access functions optional.
  drm: Make Legacy Context access functions optional.

 drivers/gpu/drm/drm_context.c         | 50 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_drv.c             | 12 +++++----
 drivers/gpu/drm/drm_lock.c            | 18 +++++++++++--
 drivers/gpu/drm/i915/i915_dma.c       |  3 +++
 drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
 include/drm/drmP.h                    | 23 ++++++++--------
 include/uapi/drm/i915_drm.h           |  1 +
 7 files changed, 87 insertions(+), 23 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm: Kernel Crash in drm_unlock

[Peter Antoine]

This patch fixes a possible kernel crash when drm_unlock (DRM_IOCTL_UNLOCK)
is called by a application that has not had a lock created by it. This
crash can be caused by any application from all users.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_lock.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index f861361..070dd5d 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -159,6 +159,14 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
 		return -EINVAL;
 	}
 
+	if (!master->lock.hw_lock) {
+		DRM_ERROR(
+			"Device has been unregistered. Hard exit. Process %d\n",
+			task_pid_nr(current));
+		send_sig(SIGTERM, current, 0);
+		return -EPERM;
+	}
+
 	if (drm_legacy_lock_free(&master->lock, lock->context)) {
 		/* FIXME: Should really bail out here. */
 	}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm: Fixes unsafe deference in locks.

[Peter Antoine]

This patch fixes an unsafe deference in the DRM_IOCTL_NEW_CTX. If the
ioctl is called before the lock is created or after it has been destroyed.
The code will deference a NULL pointer. This ioctl is a root ioctl so
exploitation is limited.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_context.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
index 9b23525..96350d1 100644
--- a/drivers/gpu/drm/drm_context.c
+++ b/drivers/gpu/drm/drm_context.c
@@ -277,7 +277,13 @@ static int drm_context_switch_complete(struct drm_device *dev,
 {
 	dev->last_context = new;	/* PRE/POST: This is the _only_ writer. */
 
-	if (!_DRM_LOCK_IS_HELD(file_priv->master->lock.hw_lock->lock)) {
+	if (file_priv->master->lock.hw_lock == NULL) {
+		DRM_ERROR(
+			"Device has been unregistered. Hard exit. Process %d\n",
+			task_pid_nr(current));
+		send_sig(SIGTERM, current, 0);
+		return -EPERM;
+	} else if (!_DRM_LOCK_IS_HELD(file_priv->master->lock.hw_lock->lock)) {
 		DRM_ERROR("Lock isn't held after context switch\n");
 	}
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm: Possible lock priority escalation.

[Peter Antoine]

If an application that has a driver lock created, wants the lock the
kernel context, it is not allowed to. If the call to drm_lock has a
context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
But as the DRM_LOCK_CONT bits are not part of the context id this allows
operations on the DRM_KERNEL_CONTEXT.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_context.c | 6 +++---
 drivers/gpu/drm/drm_lock.c    | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
index 96350d1..1febcd3 100644
--- a/drivers/gpu/drm/drm_context.c
+++ b/drivers/gpu/drm/drm_context.c
@@ -123,7 +123,7 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
 
 	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
 		if (pos->tag == file &&
-		    pos->handle != DRM_KERNEL_CONTEXT) {
+		    _DRM_LOCKING_CONTEXT(pos->handle) != DRM_KERNEL_CONTEXT) {
 			if (dev->driver->context_dtor)
 				dev->driver->context_dtor(dev, pos->handle);
 
@@ -342,7 +342,7 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
 	struct drm_ctx *ctx = data;
 
 	ctx->handle = drm_legacy_ctxbitmap_next(dev);
-	if (ctx->handle == DRM_KERNEL_CONTEXT) {
+	if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
 		/* Skip kernel's context and get a new one. */
 		ctx->handle = drm_legacy_ctxbitmap_next(dev);
 	}
@@ -449,7 +449,7 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
 	struct drm_ctx *ctx = data;
 
 	DRM_DEBUG("%d\n", ctx->handle);
-	if (ctx->handle != DRM_KERNEL_CONTEXT) {
+	if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
 		if (dev->driver->context_dtor)
 			dev->driver->context_dtor(dev, ctx->handle);
 		drm_legacy_ctxbitmap_free(dev, ctx->handle);
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index 070dd5d..94500930 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -63,7 +63,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
 
 	++file_priv->lock_count;
 
-	if (lock->context == DRM_KERNEL_CONTEXT) {
+	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
 		DRM_ERROR("Process %d using kernel context %d\n",
 			  task_pid_nr(current), lock->context);
 		return -EINVAL;
@@ -153,7 +153,7 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
 	struct drm_lock *lock = data;
 	struct drm_master *master = file_priv->master;
 
-	if (lock->context == DRM_KERNEL_CONTEXT) {
+	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
 		DRM_ERROR("Process %d using kernel context %d\n",
 			  task_pid_nr(current), lock->context);
 		return -EINVAL;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Peter Antoine]

As these functions are only used by one driver and there are security holes
in these functions. Make the functions optional.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_lock.c            |  6 ++++++
 drivers/gpu/drm/i915/i915_dma.c       |  3 +++
 drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
 include/drm/drmP.h                    | 23 ++++++++++++-----------
 include/uapi/drm/i915_drm.h           |  1 +
 5 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index 94500930..b61d4c7 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
 	struct drm_master *master = file_priv->master;
 	int ret = 0;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	++file_priv->lock_count;
 
 	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
@@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
 	struct drm_lock *lock = data;
 	struct drm_master *master = file_priv->master;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
 		DRM_ERROR("Process %d using kernel context %d\n",
 			  task_pid_nr(current), lock->context);
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e44116f..c771ef0 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		if (!value)
 			return -ENODEV;
 		break;
+	case I915_PARAM_HAS_LEGACY_CONTEXT:
+		value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 8763deb..936b423 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -940,7 +940,8 @@ static struct drm_driver
 driver_stub = {
 	.driver_features =
 		DRIVER_USE_AGP |
-		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
+		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
+		DRIVER_KMS_LEGACY_CONTEXT,
 
 	.load = nouveau_drm_load,
 	.unload = nouveau_drm_unload,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 62c40777..367e42f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -137,17 +137,18 @@ void drm_err(const char *format, ...);
 /*@{*/
 
 /* driver capabilities and requirements mask */
-#define DRIVER_USE_AGP     0x1
-#define DRIVER_PCI_DMA     0x8
-#define DRIVER_SG          0x10
-#define DRIVER_HAVE_DMA    0x20
-#define DRIVER_HAVE_IRQ    0x40
-#define DRIVER_IRQ_SHARED  0x80
-#define DRIVER_GEM         0x1000
-#define DRIVER_MODESET     0x2000
-#define DRIVER_PRIME       0x4000
-#define DRIVER_RENDER      0x8000
-#define DRIVER_ATOMIC      0x10000
+#define DRIVER_USE_AGP			0x1
+#define DRIVER_PCI_DMA			0x8
+#define DRIVER_SG			0x10
+#define DRIVER_HAVE_DMA			0x20
+#define DRIVER_HAVE_IRQ			0x40
+#define DRIVER_IRQ_SHARED		0x80
+#define DRIVER_GEM			0x1000
+#define DRIVER_MODESET			0x2000
+#define DRIVER_PRIME			0x4000
+#define DRIVER_RENDER			0x8000
+#define DRIVER_ATOMIC			0x10000
+#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
 
 /***********************************************************************/
 /** \name Macros to make printk easier */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 4851d66..0ad611a2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_REVISION              32
 #define I915_PARAM_SUBSLICE_TOTAL	 33
 #define I915_PARAM_EU_TOTAL		 34
+#define I915_PARAM_HAS_LEGACY_CONTEXT	 35
 
 typedef struct drm_i915_getparam {
 	int param;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm: Make Legacy Context access functions optional.

[Peter Antoine]

As these functions are only used by one driver and there are security holes
in these functions. Make the functions optional.

These changes are based on the two patches:
  commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
  Author: Dave Airlie <airlied@redhat.com>

And the commit that the above patch reverts:
  commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>

This should now turn off the context feature.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_context.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c     | 12 +++++++-----
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
index 1febcd3..09af26c 100644
--- a/drivers/gpu/drm/drm_context.c
+++ b/drivers/gpu/drm/drm_context.c
@@ -53,6 +53,9 @@ struct drm_ctx_list {
  */
 void drm_legacy_ctxbitmap_free(struct drm_device * dev, int ctx_handle)
 {
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 	idr_remove(&dev->ctx_idr, ctx_handle);
 	mutex_unlock(&dev->struct_mutex);
@@ -87,6 +90,9 @@ static int drm_legacy_ctxbitmap_next(struct drm_device * dev)
  */
 int drm_legacy_ctxbitmap_init(struct drm_device * dev)
 {
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	idr_init(&dev->ctx_idr);
 	return 0;
 }
@@ -101,6 +107,9 @@ int drm_legacy_ctxbitmap_init(struct drm_device * dev)
  */
 void drm_legacy_ctxbitmap_cleanup(struct drm_device * dev)
 {
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 	idr_destroy(&dev->ctx_idr);
 	mutex_unlock(&dev->struct_mutex);
@@ -119,6 +128,9 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_ctx_list *pos, *tmp;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->ctxlist_mutex);
 
 	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
@@ -161,6 +173,9 @@ int drm_legacy_getsareactx(struct drm_device *dev, void *data,
 	struct drm_local_map *map;
 	struct drm_map_list *_entry;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 
 	map = idr_find(&dev->ctx_idr, request->ctx_id);
@@ -205,6 +220,9 @@ int drm_legacy_setsareactx(struct drm_device *dev, void *data,
 	struct drm_local_map *map = NULL;
 	struct drm_map_list *r_list = NULL;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 	list_for_each_entry(r_list, &dev->maplist, head) {
 		if (r_list->map
@@ -311,6 +329,9 @@ int drm_legacy_resctx(struct drm_device *dev, void *data,
 	struct drm_ctx ctx;
 	int i;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	if (res->count >= DRM_RESERVED_CONTEXTS) {
 		memset(&ctx, 0, sizeof(ctx));
 		for (i = 0; i < DRM_RESERVED_CONTEXTS; i++) {
@@ -341,6 +362,9 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
 	struct drm_ctx_list *ctx_entry;
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	ctx->handle = drm_legacy_ctxbitmap_next(dev);
 	if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
 		/* Skip kernel's context and get a new one. */
@@ -384,6 +408,9 @@ int drm_legacy_getctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	/* This is 0, because we don't handle any context flags */
 	ctx->flags = 0;
 
@@ -406,6 +433,9 @@ int drm_legacy_switchctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	DRM_DEBUG("%d\n", ctx->handle);
 	return drm_context_switch(dev, dev->last_context, ctx->handle);
 }
@@ -426,6 +456,9 @@ int drm_legacy_newctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	DRM_DEBUG("%d\n", ctx->handle);
 	drm_context_switch_complete(dev, file_priv, ctx->handle);
 
@@ -448,6 +481,9 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	DRM_DEBUG("%d\n", ctx->handle);
 	if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
 		if (dev->driver->context_dtor)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 48f7359..dc662e8 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -596,11 +596,13 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 	if (drm_ht_create(&dev->map_hash, 12))
 		goto err_minors;
 
-	ret = drm_legacy_ctxbitmap_init(dev);
-	if (ret) {
-		DRM_ERROR("Cannot allocate memory for context bitmap.\n");
-		goto err_ht;
-	}
+	if (drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		ret = drm_legacy_ctxbitmap_init(dev);
+		if (ret) {
+			DRM_ERROR(
+				"Cannot allocate memory for context bitmap.\n");
+			goto err_ht;
+		}
 
 	if (drm_core_check_feature(dev, DRIVER_GEM)) {
 		ret = drm_gem_init(dev);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Chris Wilson]

On Thu, Apr 23, 2015 at 03:07:54PM +0100, Peter Antoine wrote:
> This patch fixes a possible kernel crash when drm_unlock (DRM_IOCTL_UNLOCK)
> is called by a application that has not had a lock created by it. This
> crash can be caused by any application from all users.

Crashing the application is still a crash...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/5] drm: Fixes unsafe deference in locks.

[Chris Wilson]

On Thu, Apr 23, 2015 at 03:07:55PM +0100, Peter Antoine wrote:
> This patch fixes an unsafe deference in the DRM_IOCTL_NEW_CTX. If the
> ioctl is called before the lock is created or after it has been destroyed.
> The code will deference a NULL pointer. This ioctl is a root ioctl so
> exploitation is limited.

You've turned an application crash into an application crash...
Just with a slightly less verbose kernel log.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

Before the patch the system required rebooting (driver crash and/or kernel panic).
Now the application gets terminated.

This follows the pattern in the other parts of this code that checks that pointer.

Peter.

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Thursday, April 23, 2015 3:20 PM
To: Antoine, Peter
Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch
Subject: Re: [Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock

On Thu, Apr 23, 2015 at 03:07:54PM +0100, Peter Antoine wrote:
> This patch fixes a possible kernel crash when drm_unlock 
> (DRM_IOCTL_UNLOCK) is called by a application that has not had a lock 
> created by it. This crash can be caused by any application from all users.

Crashing the application is still a crash...
-Chris

--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Chris Wilson]

On Thu, Apr 23, 2015 at 02:34:24PM +0000, Antoine, Peter wrote:
> Before the patch the system required rebooting (driver crash and/or kernel panic).
> Now the application gets terminated.

It should have an GPF which should not have required a reboot, but
terminated the application. Unless you set it to automatically reboot.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm: Make Legacy Context access functions optional.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6256
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  318/318              318/318
IVB                                  341/341              341/341
BYT                                  287/287              287/287
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

I picked up this work due to the following Jira ticket created by the
security team (on Android) and was asked to give it a second look and
found a few more issues with the hw lock code.

https://jira01.devtools.intel.com/browse/GMINL-5388
I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)

It also stops Linux as it kills the driver, I guess it might be possible
to reload the gfx driver. On a unpatched system the test that is
included in the issue or the igt test that has been posted for the issue
will show the problem.

I ran the test on an unpatched system here and the gui stopped and the
keyboard stopped responding, so I rebooted. With the patched system I
did not need to reboot.

Should I change the SIGTERM to SIGSEGV, not quite the same thing but
tooling is better at handling a segfault than a SIGTERM and the
application that calls this IOCTL is using an uninitialised hw lock so
it is kind of the same as differencing an uninitialised pointer (kind
of). Or, I could just remove it, but the bug has been in the code for at
least two years (and known about), and I would guess that any code that
is calling this is fuzzing the IOCTLs (as this is how the security team
found it) and we should reward them with a application exit.

Peter. 


On Thu, 2015-04-23 at 15:39 +0100, Chris Wilson wrote:
> On Thu, Apr 23, 2015 at 02:34:24PM +0000, Antoine, Peter wrote:
> > Before the patch the system required rebooting (driver crash and/or kernel panic).
> > Now the application gets terminated.
> 
> It should have an GPF which should not have required a reboot, but
> terminated the application. Unless you set it to automatically reboot.
> -Chris
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/5] drm: Possible lock priority escalation.

[Ville Syrjälä]

On Thu, Apr 23, 2015 at 03:07:56PM +0100, Peter Antoine wrote:
> If an application that has a driver lock created, wants the lock the
> kernel context, it is not allowed to. If the call to drm_lock has a
> context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
> then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
> But as the DRM_LOCK_CONT bits are not part of the context id this allows
> operations on the DRM_KERNEL_CONTEXT.
> 
> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/drm_context.c | 6 +++---
>  drivers/gpu/drm/drm_lock.c    | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> index 96350d1..1febcd3 100644
> --- a/drivers/gpu/drm/drm_context.c
> +++ b/drivers/gpu/drm/drm_context.c
> @@ -123,7 +123,7 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
>  
>  	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
>  		if (pos->tag == file &&
> -		    pos->handle != DRM_KERNEL_CONTEXT) {
> +		    _DRM_LOCKING_CONTEXT(pos->handle) != DRM_KERNEL_CONTEXT) {
>  			if (dev->driver->context_dtor)
>  				dev->driver->context_dtor(dev, pos->handle);
>  
> @@ -342,7 +342,7 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
>  	struct drm_ctx *ctx = data;
>  
>  	ctx->handle = drm_legacy_ctxbitmap_next(dev);
> -	if (ctx->handle == DRM_KERNEL_CONTEXT) {
> +	if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
>  		/* Skip kernel's context and get a new one. */
>  		ctx->handle = drm_legacy_ctxbitmap_next(dev);
>  	}
> @@ -449,7 +449,7 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
>  	struct drm_ctx *ctx = data;
>  
>  	DRM_DEBUG("%d\n", ctx->handle);
> -	if (ctx->handle != DRM_KERNEL_CONTEXT) {
> +	if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
>  		if (dev->driver->context_dtor)
>  			dev->driver->context_dtor(dev, ctx->handle);
>  		drm_legacy_ctxbitmap_free(dev, ctx->handle);

How about just fixing the end parameter passed to idr_alloc()? AFAICS
that would take care of the context code.

Well, there are a few more issues with the code:
- not properly checking for negative return value from idr_alloc()
- leaking the ctx id on kmalloc() error
- pointless check for idr_alloc() returning 0 even though the min is 1

> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index 070dd5d..94500930 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -63,7 +63,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>  
>  	++file_priv->lock_count;

While you're poking around this dungeopn, maybe you can kill lock_count?
We never seem to decrement it, and it's only checked in drm_legacy_i_have_hw_lock().

>  
> -	if (lock->context == DRM_KERNEL_CONTEXT) {
> +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
>  		DRM_ERROR("Process %d using kernel context %d\n",
>  			  task_pid_nr(current), lock->context);
>  		return -EINVAL;
> @@ -153,7 +153,7 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
>  	struct drm_lock *lock = data;
>  	struct drm_master *master = file_priv->master;
>  
> -	if (lock->context == DRM_KERNEL_CONTEXT) {
> +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
>  		DRM_ERROR("Process %d using kernel context %d\n",
>  			  task_pid_nr(current), lock->context);
>  		return -EINVAL;

These two changes look OK to me.

> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Ville Syrjälä]

On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> As these functions are only used by one driver and there are security holes
> in these functions. Make the functions optional.
> 
> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/drm_lock.c            |  6 ++++++
>  drivers/gpu/drm/i915/i915_dma.c       |  3 +++
>  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
>  include/drm/drmP.h                    | 23 ++++++++++++-----------
>  include/uapi/drm/i915_drm.h           |  1 +
>  5 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index 94500930..b61d4c7 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>  	struct drm_master *master = file_priv->master;
>  	int ret = 0;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	++file_priv->lock_count;
>  
>  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> @@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
>  	struct drm_lock *lock = data;
>  	struct drm_master *master = file_priv->master;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
>  		DRM_ERROR("Process %d using kernel context %d\n",
>  			  task_pid_nr(current), lock->context);
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e44116f..c771ef0 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		if (!value)
>  			return -ENODEV;
>  		break;
> +	case I915_PARAM_HAS_LEGACY_CONTEXT:
> +		value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> +		break;

Seems pointless to add a parameter that'll always be false.

>  	default:
>  		DRM_DEBUG("Unknown parameter %d\n", param->param);
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 8763deb..936b423 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -940,7 +940,8 @@ static struct drm_driver
>  driver_stub = {
>  	.driver_features =
>  		DRIVER_USE_AGP |
> -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> +		DRIVER_KMS_LEGACY_CONTEXT,

Why is this here? AFAICS only the via driver cares about legacy
contexts, and only dri1 drivers care about the hw lock.

>  
>  	.load = nouveau_drm_load,
>  	.unload = nouveau_drm_unload,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 62c40777..367e42f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);
>  /*@{*/
>  
>  /* driver capabilities and requirements mask */
> -#define DRIVER_USE_AGP     0x1
> -#define DRIVER_PCI_DMA     0x8
> -#define DRIVER_SG          0x10
> -#define DRIVER_HAVE_DMA    0x20
> -#define DRIVER_HAVE_IRQ    0x40
> -#define DRIVER_IRQ_SHARED  0x80
> -#define DRIVER_GEM         0x1000
> -#define DRIVER_MODESET     0x2000
> -#define DRIVER_PRIME       0x4000
> -#define DRIVER_RENDER      0x8000
> -#define DRIVER_ATOMIC      0x10000
> +#define DRIVER_USE_AGP			0x1
> +#define DRIVER_PCI_DMA			0x8
> +#define DRIVER_SG			0x10
> +#define DRIVER_HAVE_DMA			0x20
> +#define DRIVER_HAVE_IRQ			0x40
> +#define DRIVER_IRQ_SHARED		0x80
> +#define DRIVER_GEM			0x1000
> +#define DRIVER_MODESET			0x2000
> +#define DRIVER_PRIME			0x4000
> +#define DRIVER_RENDER			0x8000
> +#define DRIVER_ATOMIC			0x10000
> +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000

Why is there KMS in the name?

I was thinking just checking for GEM, but I think there was some
gem+dri1 userland for i915 at some point in time. ums and dri1 are
now dead as far as i915 is concerned, so in theory it should be fine.
But I'm not sure if some other driver might have the same baggage.

I suppose one option would be to check for MODESET instead. kms+dri1
doesn't sound like an entirely sane combination to me.

>  
>  /***********************************************************************/
>  /** \name Macros to make printk easier */
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 4851d66..0ad611a2 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_REVISION              32
>  #define I915_PARAM_SUBSLICE_TOTAL	 33
>  #define I915_PARAM_EU_TOTAL		 34
> +#define I915_PARAM_HAS_LEGACY_CONTEXT	 35
>  
>  typedef struct drm_i915_getparam {
>  	int param;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Antoine, Peter]

Hi,

(replies inline)

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Monday, April 27, 2015 6:04 PM
To: Antoine, Peter
Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch
Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.

On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> As these functions are only used by one driver and there are security 
> holes in these functions. Make the functions optional.
> 
> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/drm_lock.c            |  6 ++++++
>  drivers/gpu/drm/i915/i915_dma.c       |  3 +++
>  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
>  include/drm/drmP.h                    | 23 ++++++++++++-----------
>  include/uapi/drm/i915_drm.h           |  1 +
>  5 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> index 94500930..b61d4c7 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>  	struct drm_master *master = file_priv->master;
>  	int ret = 0;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	++file_priv->lock_count;
>  
>  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@ 
> -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
>  	struct drm_lock *lock = data;
>  	struct drm_master *master = file_priv->master;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
>  		DRM_ERROR("Process %d using kernel context %d\n",
>  			  task_pid_nr(current), lock->context); diff --git 
> a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c 
> index e44116f..c771ef0 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		if (!value)
>  			return -ENODEV;
>  		break;
> +	case I915_PARAM_HAS_LEGACY_CONTEXT:
> +		value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> +		break;

Seems pointless to add a parameter that'll always be false.

There is some history to these changes, the HW_LOCK functions were removed previously but causes an issue with the Nouveau drivers. So that the functions where put back in. So the parameter has been added to allow for that driver to turn the legacy context on as it is needed. 

>  	default:
>  		DRM_DEBUG("Unknown parameter %d\n", param->param);
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 8763deb..936b423 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -940,7 +940,8 @@ static struct drm_driver  driver_stub = {
>  	.driver_features =
>  		DRIVER_USE_AGP |
> -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> +		DRIVER_KMS_LEGACY_CONTEXT,

Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.

See above.
>  
>  	.load = nouveau_drm_load,
>  	.unload = nouveau_drm_unload,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 
> 62c40777..367e42f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);  /*@{*/
>  
>  /* driver capabilities and requirements mask */
> -#define DRIVER_USE_AGP     0x1
> -#define DRIVER_PCI_DMA     0x8
> -#define DRIVER_SG          0x10
> -#define DRIVER_HAVE_DMA    0x20
> -#define DRIVER_HAVE_IRQ    0x40
> -#define DRIVER_IRQ_SHARED  0x80
> -#define DRIVER_GEM         0x1000
> -#define DRIVER_MODESET     0x2000
> -#define DRIVER_PRIME       0x4000
> -#define DRIVER_RENDER      0x8000
> -#define DRIVER_ATOMIC      0x10000
> +#define DRIVER_USE_AGP			0x1
> +#define DRIVER_PCI_DMA			0x8
> +#define DRIVER_SG			0x10
> +#define DRIVER_HAVE_DMA			0x20
> +#define DRIVER_HAVE_IRQ			0x40
> +#define DRIVER_IRQ_SHARED		0x80
> +#define DRIVER_GEM			0x1000
> +#define DRIVER_MODESET			0x2000
> +#define DRIVER_PRIME			0x4000
> +#define DRIVER_RENDER			0x8000
> +#define DRIVER_ATOMIC			0x10000
> +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000

Why is there KMS in the name?

By suggestion of Daniel.

I was thinking just checking for GEM, but I think there was some
gem+dri1 userland for i915 at some point in time. ums and dri1 are
now dead as far as i915 is concerned, so in theory it should be fine.
But I'm not sure if some other driver might have the same baggage.

Other drivers have the same baggage.

I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.

Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.

Peter.

>  
>  
> /*********************************************************************
> **/
>  /** \name Macros to make printk easier */ diff --git 
> a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 
> 4851d66..0ad611a2 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_REVISION              32
>  #define I915_PARAM_SUBSLICE_TOTAL	 33
>  #define I915_PARAM_EU_TOTAL		 34
> +#define I915_PARAM_HAS_LEGACY_CONTEXT	 35
>  
>  typedef struct drm_i915_getparam {
>  	int param;
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Dave Gordon]

On 24/04/15 06:52, Antoine, Peter wrote:
> I picked up this work due to the following Jira ticket created by the
> security team (on Android) and was asked to give it a second look and
> found a few more issues with the hw lock code.
> 
> https://jira01.devtools.intel.com/browse/GMINL-5388
> I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
> 
> It also stops Linux as it kills the driver, I guess it might be possible
> to reload the gfx driver. On a unpatched system the test that is
> included in the issue or the igt test that has been posted for the issue
> will show the problem.
> 
> I ran the test on an unpatched system here and the gui stopped and the
> keyboard stopped responding, so I rebooted. With the patched system I
> did not need to reboot.
> 
> Should I change the SIGTERM to SIGSEGV, not quite the same thing but
> tooling is better at handling a segfault than a SIGTERM and the
> application that calls this IOCTL is using an uninitialised hw lock so
> it is kind of the same as differencing an uninitialised pointer (kind
> of). Or, I could just remove it, but the bug has been in the code for at
> least two years (and known about), and I would guess that any code that
> is calling this is fuzzing the IOCTLs (as this is how the security team
> found it) and we should reward them with a application exit.
> 
> Peter. 

SIGSEGV would be a better choice.

SIGTERM is normally sent by a user -- it's the default signal sent by
kill(1). It's also commonly used to tell a long-running daemon process
to tidy up and exit cleanly.

SIGSEGV commonly means "you accessed something that doesn't exist/isn't
mapped/you don't have permissions for". There are specific subcases that
can be indicated via the siginfo data; this is from the sigaction(1)
manpage:

    The following values can be placed in si_code for a SIGSEGV signal:

        SEGV_MAPERR    address not mapped to object

        SEGV_ACCERR    invalid permissions for mapped object

SIGBUS would also be a possibility but that's generally taken to mean
that an access got all the way to some physical bus and then faulted,
whereas SIGSEGV suggests the access was rejected during the
virtual-to-physical mapping process.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[chris]

On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
> On 24/04/15 06:52, Antoine, Peter wrote:
> > I picked up this work due to the following Jira ticket created by the
> > security team (on Android) and was asked to give it a second look and
> > found a few more issues with the hw lock code.
> > 
> > https://jira01.devtools.intel.com/browse/GMINL-5388
> > I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
> > 
> > It also stops Linux as it kills the driver, I guess it might be possible
> > to reload the gfx driver. On a unpatched system the test that is
> > included in the issue or the igt test that has been posted for the issue
> > will show the problem.
> > 
> > I ran the test on an unpatched system here and the gui stopped and the
> > keyboard stopped responding, so I rebooted. With the patched system I
> > did not need to reboot.
> > 
> > Should I change the SIGTERM to SIGSEGV, not quite the same thing but
> > tooling is better at handling a segfault than a SIGTERM and the
> > application that calls this IOCTL is using an uninitialised hw lock so
> > it is kind of the same as differencing an uninitialised pointer (kind
> > of). Or, I could just remove it, but the bug has been in the code for at
> > least two years (and known about), and I would guess that any code that
> > is calling this is fuzzing the IOCTLs (as this is how the security team
> > found it) and we should reward them with a application exit.
> > 
> > Peter. 
> 
> SIGSEGV would be a better choice.
> 
> SIGTERM is normally sent by a user -- it's the default signal sent by
> kill(1). It's also commonly used to tell a long-running daemon process
> to tidy up and exit cleanly.
> 
> SIGSEGV commonly means "you accessed something that doesn't exist/isn't
> mapped/you don't have permissions for". There are specific subcases that
> can be indicated via the siginfo data; this is from the sigaction(1)
> manpage:
> 
>     The following values can be placed in si_code for a SIGSEGV signal:
> 
>         SEGV_MAPERR    address not mapped to object
> 
>         SEGV_ACCERR    invalid permissions for mapped object
> 
> SIGBUS would also be a possibility but that's generally taken to mean
> that an access got all the way to some physical bus and then faulted,
> whereas SIGSEGV suggests the access was rejected during the
> virtual-to-physical mapping process.

None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Ville Syrjälä]

On Tue, Apr 28, 2015 at 05:52:20AM +0000, Antoine, Peter wrote:
> Hi,
> 
> (replies inline)
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Monday, April 27, 2015 6:04 PM
> To: Antoine, Peter
> Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
> 
> On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> > As these functions are only used by one driver and there are security 
> > holes in these functions. Make the functions optional.
> > 
> > Issue: VIZ-5485
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > ---
> >  drivers/gpu/drm/drm_lock.c            |  6 ++++++
> >  drivers/gpu/drm/i915/i915_dma.c       |  3 +++
> >  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
> >  include/drm/drmP.h                    | 23 ++++++++++++-----------
> >  include/uapi/drm/i915_drm.h           |  1 +
> >  5 files changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> > index 94500930..b61d4c7 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> >  	struct drm_master *master = file_priv->master;
> >  	int ret = 0;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > +		return -EINVAL;
> > +
> >  	++file_priv->lock_count;
> >  
> >  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@ 
> > -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> >  	struct drm_lock *lock = data;
> >  	struct drm_master *master = file_priv->master;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > +		return -EINVAL;
> > +
> >  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> >  		DRM_ERROR("Process %d using kernel context %d\n",
> >  			  task_pid_nr(current), lock->context); diff --git 
> > a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c 
> > index e44116f..c771ef0 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >  		if (!value)
> >  			return -ENODEV;
> >  		break;
> > +	case I915_PARAM_HAS_LEGACY_CONTEXT:
> > +		value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> > +		break;
> 
> Seems pointless to add a parameter that'll always be false.
> 
> There is some history to these changes, the HW_LOCK functions were removed previously but causes an issue with the Nouveau drivers. So that the functions where put back in. So the parameter has been added to allow for that driver to turn the legacy context on as it is needed. 
> 
> >  	default:
> >  		DRM_DEBUG("Unknown parameter %d\n", param->param);
> >  		return -EINVAL;
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 8763deb..936b423 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -940,7 +940,8 @@ static struct drm_driver  driver_stub = {
> >  	.driver_features =
> >  		DRIVER_USE_AGP |
> > -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> > +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> > +		DRIVER_KMS_LEGACY_CONTEXT,
> 
> Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.
> 
> See above.
> >  
> >  	.load = nouveau_drm_load,
> >  	.unload = nouveau_drm_unload,
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 
> > 62c40777..367e42f 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);  /*@{*/
> >  
> >  /* driver capabilities and requirements mask */
> > -#define DRIVER_USE_AGP     0x1
> > -#define DRIVER_PCI_DMA     0x8
> > -#define DRIVER_SG          0x10
> > -#define DRIVER_HAVE_DMA    0x20
> > -#define DRIVER_HAVE_IRQ    0x40
> > -#define DRIVER_IRQ_SHARED  0x80
> > -#define DRIVER_GEM         0x1000
> > -#define DRIVER_MODESET     0x2000
> > -#define DRIVER_PRIME       0x4000
> > -#define DRIVER_RENDER      0x8000
> > -#define DRIVER_ATOMIC      0x10000
> > +#define DRIVER_USE_AGP			0x1
> > +#define DRIVER_PCI_DMA			0x8
> > +#define DRIVER_SG			0x10
> > +#define DRIVER_HAVE_DMA			0x20
> > +#define DRIVER_HAVE_IRQ			0x40
> > +#define DRIVER_IRQ_SHARED		0x80
> > +#define DRIVER_GEM			0x1000
> > +#define DRIVER_MODESET			0x2000
> > +#define DRIVER_PRIME			0x4000
> > +#define DRIVER_RENDER			0x8000
> > +#define DRIVER_ATOMIC			0x10000
> > +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> 
> Why is there KMS in the name?
> 
> By suggestion of Daniel.
> 
> I was thinking just checking for GEM, but I think there was some
> gem+dri1 userland for i915 at some point in time. ums and dri1 are
> now dead as far as i915 is concerned, so in theory it should be fine.
> But I'm not sure if some other driver might have the same baggage.
> 
> Other drivers have the same baggage.
> 
> I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
> 
> Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.

Reference?

> 
> Peter.
> 
> >  
> >  
> > /*********************************************************************
> > **/
> >  /** \name Macros to make printk easier */ diff --git 
> > a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 
> > 4851d66..0ad611a2 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
> >  #define I915_PARAM_REVISION              32
> >  #define I915_PARAM_SUBSLICE_TOTAL	 33
> >  #define I915_PARAM_EU_TOTAL		 34
> > +#define I915_PARAM_HAS_LEGACY_CONTEXT	 35
> >  
> >  typedef struct drm_i915_getparam {
> >  	int param;
> > --
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Antoine, Peter]

reply at end.

On Tue, 2015-04-28 at 13:40 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 05:52:20AM +0000, Antoine, Peter wrote:
> > Hi,
> > 
> > (replies inline)
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> > Sent: Monday, April 27, 2015 6:04 PM
> > To: Antoine, Peter
> > Cc: intel-gfx@lists.freedesktop.org; airlied@redhat.com; dri-devel@lists.freedesktop.org; daniel.vetter@ffwll.ch
> > Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions optional.
> > 
> > On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> > > As these functions are only used by one driver and there are security 
> > > holes in these functions. Make the functions optional.
> > > 
> > > Issue: VIZ-5485
> > > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_lock.c            |  6 ++++++
> > >  drivers/gpu/drm/i915/i915_dma.c       |  3 +++
> > >  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
> > >  include/drm/drmP.h                    | 23 ++++++++++++-----------
> > >  include/uapi/drm/i915_drm.h           |  1 +
> > >  5 files changed, 24 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c 
> > > index 94500930..b61d4c7 100644
> > > --- a/drivers/gpu/drm/drm_lock.c
> > > +++ b/drivers/gpu/drm/drm_lock.c
> > > @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> > >  	struct drm_master *master = file_priv->master;
> > >  	int ret = 0;
> > >  
> > > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > +		return -EINVAL;
> > > +
> > >  	++file_priv->lock_count;
> > >  
> > >  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@ 
> > > -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> > >  	struct drm_lock *lock = data;
> > >  	struct drm_master *master = file_priv->master;
> > >  
> > > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > +		return -EINVAL;
> > > +
> > >  	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > >  		DRM_ERROR("Process %d using kernel context %d\n",
> > >  			  task_pid_nr(current), lock->context); diff --git 
> > > a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c 
> > > index e44116f..c771ef0 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> > >  		if (!value)
> > >  			return -ENODEV;
> > >  		break;
> > > +	case I915_PARAM_HAS_LEGACY_CONTEXT:
> > > +		value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> > > +		break;
> > 
> > Seems pointless to add a parameter that'll always be false.
> > 
> > There is some history to these changes, the HW_LOCK functions were removed previously but causes an issue with the Nouveau drivers. So that the functions where put back in. So the parameter has been added to allow for that driver to turn the legacy context on as it is needed. 
> > 
> > >  	default:
> > >  		DRM_DEBUG("Unknown parameter %d\n", param->param);
> > >  		return -EINVAL;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> > > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > index 8763deb..936b423 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > @@ -940,7 +940,8 @@ static struct drm_driver  driver_stub = {
> > >  	.driver_features =
> > >  		DRIVER_USE_AGP |
> > > -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> > > +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> > > +		DRIVER_KMS_LEGACY_CONTEXT,
> > 
> > Why is this here? AFAICS only the via driver cares about legacy contexts, and only dri1 drivers care about the hw lock.
> > 
> > See above.
> > >  
> > >  	.load = nouveau_drm_load,
> > >  	.unload = nouveau_drm_unload,
> > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 
> > > 62c40777..367e42f 100644
> > > --- a/include/drm/drmP.h
> > > +++ b/include/drm/drmP.h
> > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);  /*@{*/
> > >  
> > >  /* driver capabilities and requirements mask */
> > > -#define DRIVER_USE_AGP     0x1
> > > -#define DRIVER_PCI_DMA     0x8
> > > -#define DRIVER_SG          0x10
> > > -#define DRIVER_HAVE_DMA    0x20
> > > -#define DRIVER_HAVE_IRQ    0x40
> > > -#define DRIVER_IRQ_SHARED  0x80
> > > -#define DRIVER_GEM         0x1000
> > > -#define DRIVER_MODESET     0x2000
> > > -#define DRIVER_PRIME       0x4000
> > > -#define DRIVER_RENDER      0x8000
> > > -#define DRIVER_ATOMIC      0x10000
> > > +#define DRIVER_USE_AGP			0x1
> > > +#define DRIVER_PCI_DMA			0x8
> > > +#define DRIVER_SG			0x10
> > > +#define DRIVER_HAVE_DMA			0x20
> > > +#define DRIVER_HAVE_IRQ			0x40
> > > +#define DRIVER_IRQ_SHARED		0x80
> > > +#define DRIVER_GEM			0x1000
> > > +#define DRIVER_MODESET			0x2000
> > > +#define DRIVER_PRIME			0x4000
> > > +#define DRIVER_RENDER			0x8000
> > > +#define DRIVER_ATOMIC			0x10000
> > > +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> > 
> > Why is there KMS in the name?
> > 
> > By suggestion of Daniel.
> > 
> > I was thinking just checking for GEM, but I think there was some
> > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > now dead as far as i915 is concerned, so in theory it should be fine.
> > But I'm not sure if some other driver might have the same baggage.
> > 
> > Other drivers have the same baggage.
> > 
> > I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
> > 
> > Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
> 
> Reference?

From the next commit [5/5] as it is the one that actually turns off the
functions that were turned off before.

These changes are based on the two patches:
  commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
  Author: Dave Airlie <airlied@redhat.com>

And the commit that the above patch reverts:
  commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> > 
> > Peter.
> > 
> > >  
> > >  
> > > /*********************************************************************
> > > **/
> > >  /** \name Macros to make printk easier */ diff --git 
> > > a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 
> > > 4851d66..0ad611a2 100644
> > > --- a/include/uapi/drm/i915_drm.h
> > > +++ b/include/uapi/drm/i915_drm.h
> > > @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
> > >  #define I915_PARAM_REVISION              32
> > >  #define I915_PARAM_SUBSLICE_TOTAL	 33
> > >  #define I915_PARAM_EU_TOTAL		 34
> > > +#define I915_PARAM_HAS_LEGACY_CONTEXT	 35
> > >  
> > >  typedef struct drm_i915_getparam {
> > >  	int param;
> > > --
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Ville Syrjälä]

On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
> > > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 
> > > > 62c40777..367e42f 100644
> > > > --- a/include/drm/drmP.h
> > > > +++ b/include/drm/drmP.h
> > > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);  /*@{*/
> > > >  
> > > >  /* driver capabilities and requirements mask */
> > > > -#define DRIVER_USE_AGP     0x1
> > > > -#define DRIVER_PCI_DMA     0x8
> > > > -#define DRIVER_SG          0x10
> > > > -#define DRIVER_HAVE_DMA    0x20
> > > > -#define DRIVER_HAVE_IRQ    0x40
> > > > -#define DRIVER_IRQ_SHARED  0x80
> > > > -#define DRIVER_GEM         0x1000
> > > > -#define DRIVER_MODESET     0x2000
> > > > -#define DRIVER_PRIME       0x4000
> > > > -#define DRIVER_RENDER      0x8000
> > > > -#define DRIVER_ATOMIC      0x10000
> > > > +#define DRIVER_USE_AGP			0x1
> > > > +#define DRIVER_PCI_DMA			0x8
> > > > +#define DRIVER_SG			0x10
> > > > +#define DRIVER_HAVE_DMA			0x20
> > > > +#define DRIVER_HAVE_IRQ			0x40
> > > > +#define DRIVER_IRQ_SHARED		0x80
> > > > +#define DRIVER_GEM			0x1000
> > > > +#define DRIVER_MODESET			0x2000
> > > > +#define DRIVER_PRIME			0x4000
> > > > +#define DRIVER_RENDER			0x8000
> > > > +#define DRIVER_ATOMIC			0x10000
> > > > +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> > > 
> > > Why is there KMS in the name?
> > > 
> > > By suggestion of Daniel.
> > > 
> > > I was thinking just checking for GEM, but I think there was some
> > > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > > now dead as far as i915 is concerned, so in theory it should be fine.
> > > But I'm not sure if some other driver might have the same baggage.
> > > 
> > > Other drivers have the same baggage.
> > > 
> > > I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
> > > 
> > > Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
> > 
> > Reference?
> 
> From the next commit [5/5] as it is the one that actually turns off the
> functions that were turned off before.
> 
> These changes are based on the two patches:
>   commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
>   Author: Dave Airlie <airlied@redhat.com>
> 
> And the commit that the above patch reverts:
>   commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>

Looking at ancient libdrm sources makes me think nouveau just used to
create and destroy the context, but not actually use it for anything.
So nopping out the ioctls should be good enough AFAICS. Or am I missing
something?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Antoine, Peter]

On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
> > > > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 
> > > > > 62c40777..367e42f 100644
> > > > > --- a/include/drm/drmP.h
> > > > > +++ b/include/drm/drmP.h
> > > > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);  /*@{*/
> > > > >  
> > > > >  /* driver capabilities and requirements mask */
> > > > > -#define DRIVER_USE_AGP     0x1
> > > > > -#define DRIVER_PCI_DMA     0x8
> > > > > -#define DRIVER_SG          0x10
> > > > > -#define DRIVER_HAVE_DMA    0x20
> > > > > -#define DRIVER_HAVE_IRQ    0x40
> > > > > -#define DRIVER_IRQ_SHARED  0x80
> > > > > -#define DRIVER_GEM         0x1000
> > > > > -#define DRIVER_MODESET     0x2000
> > > > > -#define DRIVER_PRIME       0x4000
> > > > > -#define DRIVER_RENDER      0x8000
> > > > > -#define DRIVER_ATOMIC      0x10000
> > > > > +#define DRIVER_USE_AGP			0x1
> > > > > +#define DRIVER_PCI_DMA			0x8
> > > > > +#define DRIVER_SG			0x10
> > > > > +#define DRIVER_HAVE_DMA			0x20
> > > > > +#define DRIVER_HAVE_IRQ			0x40
> > > > > +#define DRIVER_IRQ_SHARED		0x80
> > > > > +#define DRIVER_GEM			0x1000
> > > > > +#define DRIVER_MODESET			0x2000
> > > > > +#define DRIVER_PRIME			0x4000
> > > > > +#define DRIVER_RENDER			0x8000
> > > > > +#define DRIVER_ATOMIC			0x10000
> > > > > +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> > > > 
> > > > Why is there KMS in the name?
> > > > 
> > > > By suggestion of Daniel.
> > > > 
> > > > I was thinking just checking for GEM, but I think there was some
> > > > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > > > now dead as far as i915 is concerned, so in theory it should be fine.
> > > > But I'm not sure if some other driver might have the same baggage.
> > > > 
> > > > Other drivers have the same baggage.
> > > > 
> > > > I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
> > > > 
> > > > Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
> > > 
> > > Reference?
> > 
> > From the next commit [5/5] as it is the one that actually turns off the
> > functions that were turned off before.
> > 
> > These changes are based on the two patches:
> >   commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
> >   Author: Dave Airlie <airlied@redhat.com>
> > 
> > And the commit that the above patch reverts:
> >   commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> >   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Looking at ancient libdrm sources makes me think nouveau just used to
> create and destroy the context, but not actually use it for anything.
> So nopping out the ioctls should be good enough AFAICS. Or am I missing
> something?
> 

An old version of libdrm that still requires support needs them, it's
the reason that David Airlie reverted the patch that Daniel did to
remove the functions. Do they still need support, I don't know? David
Airlie is on the cc list.

A discussion was had and this was the way that it was suggested it be
done. This seems a good half-way house, the actual security holes that
have been found have been fixed and the functions (that seem to have a
lot more security issues in them) are turned off for the drivers that
don't use them, and if a driver does require them, it will be a one line
change to reintroduce them. Are we carrying code we don't need to
support, probably.

Peter.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Dave Gordon]

On 28/04/15 10:21, Dave Gordon wrote:
> On 24/04/15 06:52, Antoine, Peter wrote:
>> I picked up this work due to the following Jira ticket created by the
>> security team (on Android) and was asked to give it a second look and
>> found a few more issues with the hw lock code.
>>
>> https://jira01.devtools.intel.com/browse/GMINL-5388
>> I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
>>
>> It also stops Linux as it kills the driver, I guess it might be possible
>> to reload the gfx driver. On a unpatched system the test that is
>> included in the issue or the igt test that has been posted for the issue
>> will show the problem.
>>
>> I ran the test on an unpatched system here and the gui stopped and the
>> keyboard stopped responding, so I rebooted. With the patched system I
>> did not need to reboot.
>>
>> Should I change the SIGTERM to SIGSEGV, not quite the same thing but
>> tooling is better at handling a segfault than a SIGTERM and the
>> application that calls this IOCTL is using an uninitialised hw lock so
>> it is kind of the same as differencing an uninitialised pointer (kind
>> of). Or, I could just remove it, but the bug has been in the code for at
>> least two years (and known about), and I would guess that any code that
>> is calling this is fuzzing the IOCTLs (as this is how the security team
>> found it) and we should reward them with a application exit.
>>
>> Peter. 
> 
> SIGSEGV would be a better choice.

[snip]

Nope, I've changed my mind about this. I thought the problematic case
was just a process releasing the lock without having acquired it, but on
further examination it's really that the DRM master process has gone
away or otherwise deleted (or never created?) the lock. And THEN the
(non-master?) process tries to release the (now) non-existent lock.

But more importantly, there's existing code in the acquire-lock path
that sends SIGTERM for this case, so obviously the release-lock code
should be as similar as possible.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Daniel Vetter]

On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris@chris-wilson.co.uk wrote:
> On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
> > On 24/04/15 06:52, Antoine, Peter wrote:
> > > I picked up this work due to the following Jira ticket created by the
> > > security team (on Android) and was asked to give it a second look and
> > > found a few more issues with the hw lock code.
> > > 
> > > https://jira01.devtools.intel.com/browse/GMINL-5388
> > > I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
> > > 
> > > It also stops Linux as it kills the driver, I guess it might be possible
> > > to reload the gfx driver. On a unpatched system the test that is
> > > included in the issue or the igt test that has been posted for the issue
> > > will show the problem.
> > > 
> > > I ran the test on an unpatched system here and the gui stopped and the
> > > keyboard stopped responding, so I rebooted. With the patched system I
> > > did not need to reboot.
> > > 
> > > Should I change the SIGTERM to SIGSEGV, not quite the same thing but
> > > tooling is better at handling a segfault than a SIGTERM and the
> > > application that calls this IOCTL is using an uninitialised hw lock so
> > > it is kind of the same as differencing an uninitialised pointer (kind
> > > of). Or, I could just remove it, but the bug has been in the code for at
> > > least two years (and known about), and I would guess that any code that
> > > is calling this is fuzzing the IOCTLs (as this is how the security team
> > > found it) and we should reward them with a application exit.
> > > 
> > > Peter. 
> > 
> > SIGSEGV would be a better choice.
> > 
> > SIGTERM is normally sent by a user -- it's the default signal sent by
> > kill(1). It's also commonly used to tell a long-running daemon process
> > to tidy up and exit cleanly.
> > 
> > SIGSEGV commonly means "you accessed something that doesn't exist/isn't
> > mapped/you don't have permissions for". There are specific subcases that
> > can be indicated via the siginfo data; this is from the sigaction(1)
> > manpage:
> > 
> >     The following values can be placed in si_code for a SIGSEGV signal:
> > 
> >         SEGV_MAPERR    address not mapped to object
> > 
> >         SEGV_ACCERR    invalid permissions for mapped object
> > 
> > SIGBUS would also be a possibility but that's generally taken to mean
> > that an access got all the way to some physical bus and then faulted,
> > whereas SIGSEGV suggests the access was rejected during the
> > virtual-to-physical mapping process.
> 
> None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.

Seconded, we really don't want to be in the business of fixing up the drm
design mistakes of the past 15 years. As long as we can fully lock out
this particular dragon when running i915 we're imo good enough. The dri1
design of a kernel shim driver cooperating with the ums driver for hw
ownership is fundamentally unfixable.

Also we can't change any of it for drivers actually using it since it'll
break them, which is a big no-go.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/5] drm: Possible lock priority escalation.

[Daniel Vetter]

On Mon, Apr 27, 2015 at 07:52:46PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 23, 2015 at 03:07:56PM +0100, Peter Antoine wrote:
> > If an application that has a driver lock created, wants the lock the
> > kernel context, it is not allowed to. If the call to drm_lock has a
> > context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
> > then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
> > But as the DRM_LOCK_CONT bits are not part of the context id this allows
> > operations on the DRM_KERNEL_CONTEXT.
> > 
> > Issue: VIZ-5485
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>

If you're touching code with drm_legacy_ prefix of in such a file you've
ended up in the horrible corners of the dri1 dungeons and should head back
out pronto ;-)

If we can actually run into this code on production i915 then we need to
improve the locks at the door of these dungeons for kms drivers, not try
to fix up the mess behind them. That's just plain impossible.

If you want to make really sure we get this right some simple drm igt
tests to make sure these codepaths are really dead for kms driver might be
good. But otherwise we really can only annotate this as wontfix in
code security issue scanners.
-Daniel

> > ---
> >  drivers/gpu/drm/drm_context.c | 6 +++---
> >  drivers/gpu/drm/drm_lock.c    | 4 ++--
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> > index 96350d1..1febcd3 100644
> > --- a/drivers/gpu/drm/drm_context.c
> > +++ b/drivers/gpu/drm/drm_context.c
> > @@ -123,7 +123,7 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
> >  
> >  	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
> >  		if (pos->tag == file &&
> > -		    pos->handle != DRM_KERNEL_CONTEXT) {
> > +		    _DRM_LOCKING_CONTEXT(pos->handle) != DRM_KERNEL_CONTEXT) {
> >  			if (dev->driver->context_dtor)
> >  				dev->driver->context_dtor(dev, pos->handle);
> >  
> > @@ -342,7 +342,7 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
> >  	struct drm_ctx *ctx = data;
> >  
> >  	ctx->handle = drm_legacy_ctxbitmap_next(dev);
> > -	if (ctx->handle == DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
> >  		/* Skip kernel's context and get a new one. */
> >  		ctx->handle = drm_legacy_ctxbitmap_next(dev);
> >  	}
> > @@ -449,7 +449,7 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
> >  	struct drm_ctx *ctx = data;
> >  
> >  	DRM_DEBUG("%d\n", ctx->handle);
> > -	if (ctx->handle != DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
> >  		if (dev->driver->context_dtor)
> >  			dev->driver->context_dtor(dev, ctx->handle);
> >  		drm_legacy_ctxbitmap_free(dev, ctx->handle);
> 
> How about just fixing the end parameter passed to idr_alloc()? AFAICS
> that would take care of the context code.
> 
> Well, there are a few more issues with the code:
> - not properly checking for negative return value from idr_alloc()
> - leaking the ctx id on kmalloc() error
> - pointless check for idr_alloc() returning 0 even though the min is 1
> 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > index 070dd5d..94500930 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -63,7 +63,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> >  
> >  	++file_priv->lock_count;
> 
> While you're poking around this dungeopn, maybe you can kill lock_count?
> We never seem to decrement it, and it's only checked in drm_legacy_i_have_hw_lock().
> 
> >  
> > -	if (lock->context == DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> >  		DRM_ERROR("Process %d using kernel context %d\n",
> >  			  task_pid_nr(current), lock->context);
> >  		return -EINVAL;
> > @@ -153,7 +153,7 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> >  	struct drm_lock *lock = data;
> >  	struct drm_master *master = file_priv->master;
> >  
> > -	if (lock->context == DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> >  		DRM_ERROR("Process %d using kernel context %d\n",
> >  			  task_pid_nr(current), lock->context);
> >  		return -EINVAL;
> 
> These two changes look OK to me.
> 
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Daniel Vetter]

On Tue, Apr 28, 2015 at 01:29:21PM +0000, Antoine, Peter wrote:
> On Tue, 2015-04-28 at 16:08 +0300, Ville Syrjälä wrote:
> > On Tue, Apr 28, 2015 at 11:29:06AM +0000, Antoine, Peter wrote:
> > > > > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 
> > > > > > 62c40777..367e42f 100644
> > > > > > --- a/include/drm/drmP.h
> > > > > > +++ b/include/drm/drmP.h
> > > > > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);  /*@{*/
> > > > > >  
> > > > > >  /* driver capabilities and requirements mask */
> > > > > > -#define DRIVER_USE_AGP     0x1
> > > > > > -#define DRIVER_PCI_DMA     0x8
> > > > > > -#define DRIVER_SG          0x10
> > > > > > -#define DRIVER_HAVE_DMA    0x20
> > > > > > -#define DRIVER_HAVE_IRQ    0x40
> > > > > > -#define DRIVER_IRQ_SHARED  0x80
> > > > > > -#define DRIVER_GEM         0x1000
> > > > > > -#define DRIVER_MODESET     0x2000
> > > > > > -#define DRIVER_PRIME       0x4000
> > > > > > -#define DRIVER_RENDER      0x8000
> > > > > > -#define DRIVER_ATOMIC      0x10000
> > > > > > +#define DRIVER_USE_AGP			0x1
> > > > > > +#define DRIVER_PCI_DMA			0x8
> > > > > > +#define DRIVER_SG			0x10
> > > > > > +#define DRIVER_HAVE_DMA			0x20
> > > > > > +#define DRIVER_HAVE_IRQ			0x40
> > > > > > +#define DRIVER_IRQ_SHARED		0x80
> > > > > > +#define DRIVER_GEM			0x1000
> > > > > > +#define DRIVER_MODESET			0x2000
> > > > > > +#define DRIVER_PRIME			0x4000
> > > > > > +#define DRIVER_RENDER			0x8000
> > > > > > +#define DRIVER_ATOMIC			0x10000
> > > > > > +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> > > > > 
> > > > > Why is there KMS in the name?
> > > > > 
> > > > > By suggestion of Daniel.
> > > > > 
> > > > > I was thinking just checking for GEM, but I think there was some
> > > > > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > > > > now dead as far as i915 is concerned, so in theory it should be fine.
> > > > > But I'm not sure if some other driver might have the same baggage.
> > > > > 
> > > > > Other drivers have the same baggage.
> > > > > 
> > > > > I suppose one option would be to check for MODESET instead. kms+dri1 doesn't sound like an entirely sane combination to me.
> > > > > 
> > > > > Can't use the MODESET as this was how it was turned off in the previous incarnation and was reverted by Dave Airle.
> > > > 
> > > > Reference?
> > > 
> > > From the next commit [5/5] as it is the one that actually turns off the
> > > functions that were turned off before.
> > > 
> > > These changes are based on the two patches:
> > >   commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
> > >   Author: Dave Airlie <airlied@redhat.com>
> > > 
> > > And the commit that the above patch reverts:
> > >   commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> > >   Author: Daniel Vetter <daniel.vetter@ffwll.ch>

These two commits definitely should be referenced from the commit message,
othwerise no one will understand the history.

> > Looking at ancient libdrm sources makes me think nouveau just used to
> > create and destroy the context, but not actually use it for anything.
> > So nopping out the ioctls should be good enough AFAICS. Or am I missing
> > something?
> > 
> 
> An old version of libdrm that still requires support needs them, it's
> the reason that David Airlie reverted the patch that Daniel did to
> remove the functions. Do they still need support, I don't know? David
> Airlie is on the cc list.
> 
> A discussion was had and this was the way that it was suggested it be
> done. This seems a good half-way house, the actual security holes that
> have been found have been fixed and the functions (that seem to have a
> lot more security issues in them) are turned off for the drivers that
> don't use them, and if a driver does require them, it will be a one line
> change to reintroduce them. Are we carrying code we don't need to
> support, probably.

Iirc it was in the ddx, and it was actually using the mmap code. Leftovers
from ums, but unfortunately X crashes if we take them away. If I recall
correctly nouveau was in staging still, but per Linus staging or not
doesn't matter when distros are shipping with the code already. I did dig
out the details way back out of curiosity, but lost them since then again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

[Dave Airlie]

>
> Iirc it was in the ddx, and it was actually using the mmap code. Leftovers
> from ums, but unfortunately X crashes if we take them away. If I recall
> correctly nouveau was in staging still, but per Linus staging or not
> doesn't matter when distros are shipping with the code already. I did dig
> out the details way back out of curiosity, but lost them since then again.
> -Daniel

There were two different nouveau problems

a) old libdrm used contexts, it called drmCreateContext and drmDestroyContext
from what I could see.

b) old DDX used DRI1 functions, DRIOpenDRMMaster in the X server
when it wanted to use open, that function added maps with locks
and mapped them.

I get the impression this is case (b).

Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Antoine, Peter]

On Mon, 2015-05-04 at 15:52 +0200, Daniel Vetter wrote:
> On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris@chris-wilson.co.uk wrote:
> > On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
> > > On 24/04/15 06:52, Antoine, Peter wrote:
> > > > I picked up this work due to the following Jira ticket created by the
> > > > security team (on Android) and was asked to give it a second look and
> > > > found a few more issues with the hw lock code.
> > > > 
> > > > https://jira01.devtools.intel.com/browse/GMINL-5388
> > > > I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
> > > > 
> > > > It also stops Linux as it kills the driver, I guess it might be possible
> > > > to reload the gfx driver. On a unpatched system the test that is
> > > > included in the issue or the igt test that has been posted for the issue
> > > > will show the problem.
> > > > 
> > > > I ran the test on an unpatched system here and the gui stopped and the
> > > > keyboard stopped responding, so I rebooted. With the patched system I
> > > > did not need to reboot.
> > > > 
> > > > Should I change the SIGTERM to SIGSEGV, not quite the same thing but
> > > > tooling is better at handling a segfault than a SIGTERM and the
> > > > application that calls this IOCTL is using an uninitialised hw lock so
> > > > it is kind of the same as differencing an uninitialised pointer (kind
> > > > of). Or, I could just remove it, but the bug has been in the code for at
> > > > least two years (and known about), and I would guess that any code that
> > > > is calling this is fuzzing the IOCTLs (as this is how the security team
> > > > found it) and we should reward them with a application exit.
> > > > 
> > > > Peter. 
> > > 
> > > SIGSEGV would be a better choice.
> > > 
> > > SIGTERM is normally sent by a user -- it's the default signal sent by
> > > kill(1). It's also commonly used to tell a long-running daemon process
> > > to tidy up and exit cleanly.
> > > 
> > > SIGSEGV commonly means "you accessed something that doesn't exist/isn't
> > > mapped/you don't have permissions for". There are specific subcases that
> > > can be indicated via the siginfo data; this is from the sigaction(1)
> > > manpage:
> > > 
> > >     The following values can be placed in si_code for a SIGSEGV signal:
> > > 
> > >         SEGV_MAPERR    address not mapped to object
> > > 
> > >         SEGV_ACCERR    invalid permissions for mapped object
> > > 
> > > SIGBUS would also be a possibility but that's generally taken to mean
> > > that an access got all the way to some physical bus and then faulted,
> > > whereas SIGSEGV suggests the access was rejected during the
> > > virtual-to-physical mapping process.
> > 
> > None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.
> 
> Seconded, we really don't want to be in the business of fixing up the drm
> design mistakes of the past 15 years. As long as we can fully lock out
> this particular dragon when running i915 we're imo good enough. The dri1
> design of a kernel shim driver cooperating with the ums driver for hw
> ownership is fundamentally unfixable.
> 
> Also we can't change any of it for drivers actually using it since it'll
> break them, which is a big no-go.
> -Daniel

I will remove it. But, If you are using this code path the driver/kernel
will have crashed. It covers a NULL pointer deference, so we are not
changing the API that anyone is actually using.

Peter. 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm: Possible lock priority escalation.

[Antoine, Peter]

On Mon, 2015-05-04 at 15:56 +0200, Daniel Vetter wrote:
> On Mon, Apr 27, 2015 at 07:52:46PM +0300, Ville Syrjälä wrote:
> > On Thu, Apr 23, 2015 at 03:07:56PM +0100, Peter Antoine wrote:
> > > If an application that has a driver lock created, wants the lock the
> > > kernel context, it is not allowed to. If the call to drm_lock has a
> > > context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
> > > then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
> > > But as the DRM_LOCK_CONT bits are not part of the context id this allows
> > > operations on the DRM_KERNEL_CONTEXT.
> > > 
> > > Issue: VIZ-5485
> > > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> 
> If you're touching code with drm_legacy_ prefix of in such a file you've
> ended up in the horrible corners of the dri1 dungeons and should head back
> out pronto ;-)
> 
> If we can actually run into this code on production i915 then we need to
> improve the locks at the door of these dungeons for kms drivers, not try
> to fix up the mess behind them. That's just plain impossible.
> 
> If you want to make really sure we get this right some simple drm igt
> tests to make sure these codepaths are really dead for kms driver might be
> good. But otherwise we really can only annotate this as wontfix in
> code security issue scanners.
> -Daniel
> 
There is a test that covers this fix. This is a simple three line fix
that stops a userspace driver locking the kernel context. Yes they are
other problems with this code, but why are they stopping this patch that
does a simple fix from going in?

I'll happily drop this patch if it causes more problems that it fixes.

Peter.

> > > ---
> > >  drivers/gpu/drm/drm_context.c | 6 +++---
> > >  drivers/gpu/drm/drm_lock.c    | 4 ++--
> > >  2 files changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> > > index 96350d1..1febcd3 100644
> > > --- a/drivers/gpu/drm/drm_context.c
> > > +++ b/drivers/gpu/drm/drm_context.c
> > > @@ -123,7 +123,7 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
> > >  
> > >  	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
> > >  		if (pos->tag == file &&
> > > -		    pos->handle != DRM_KERNEL_CONTEXT) {
> > > +		    _DRM_LOCKING_CONTEXT(pos->handle) != DRM_KERNEL_CONTEXT) {
> > >  			if (dev->driver->context_dtor)
> > >  				dev->driver->context_dtor(dev, pos->handle);
> > >  
> > > @@ -342,7 +342,7 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
> > >  	struct drm_ctx *ctx = data;
> > >  
> > >  	ctx->handle = drm_legacy_ctxbitmap_next(dev);
> > > -	if (ctx->handle == DRM_KERNEL_CONTEXT) {
> > > +	if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
> > >  		/* Skip kernel's context and get a new one. */
> > >  		ctx->handle = drm_legacy_ctxbitmap_next(dev);
> > >  	}
> > > @@ -449,7 +449,7 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
> > >  	struct drm_ctx *ctx = data;
> > >  
> > >  	DRM_DEBUG("%d\n", ctx->handle);
> > > -	if (ctx->handle != DRM_KERNEL_CONTEXT) {
> > > +	if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
> > >  		if (dev->driver->context_dtor)
> > >  			dev->driver->context_dtor(dev, ctx->handle);
> > >  		drm_legacy_ctxbitmap_free(dev, ctx->handle);
> > 
> > How about just fixing the end parameter passed to idr_alloc()? AFAICS
> > that would take care of the context code.
> > 
> > Well, there are a few more issues with the code:
> > - not properly checking for negative return value from idr_alloc()
> > - leaking the ctx id on kmalloc() error
> > - pointless check for idr_alloc() returning 0 even though the min is 1
> > 
> > > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > > index 070dd5d..94500930 100644
> > > --- a/drivers/gpu/drm/drm_lock.c
> > > +++ b/drivers/gpu/drm/drm_lock.c
> > > @@ -63,7 +63,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> > >  
> > >  	++file_priv->lock_count;
> > 
> > While you're poking around this dungeopn, maybe you can kill lock_count?
> > We never seem to decrement it, and it's only checked in drm_legacy_i_have_hw_lock().
> > 
> > >  
> > > -	if (lock->context == DRM_KERNEL_CONTEXT) {
> > > +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > >  		DRM_ERROR("Process %d using kernel context %d\n",
> > >  			  task_pid_nr(current), lock->context);
> > >  		return -EINVAL;
> > > @@ -153,7 +153,7 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> > >  	struct drm_lock *lock = data;
> > >  	struct drm_master *master = file_priv->master;
> > >  
> > > -	if (lock->context == DRM_KERNEL_CONTEXT) {
> > > +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > >  		DRM_ERROR("Process %d using kernel context %d\n",
> > >  			  task_pid_nr(current), lock->context);
> > >  		return -EINVAL;
> > 
> > These two changes look OK to me.
> > 
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Daniel Vetter]

On Tue, May 05, 2015 at 06:37:51AM +0000, Antoine, Peter wrote:
> On Mon, 2015-05-04 at 15:52 +0200, Daniel Vetter wrote:
> > On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris@chris-wilson.co.uk wrote:
> > > On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
> > > > On 24/04/15 06:52, Antoine, Peter wrote:
> > > > > I picked up this work due to the following Jira ticket created by the
> > > > > security team (on Android) and was asked to give it a second look and
> > > > > found a few more issues with the hw lock code.
> > > > > 
> > > > > https://jira01.devtools.intel.com/browse/GMINL-5388
> > > > > I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
> > > > > 
> > > > > It also stops Linux as it kills the driver, I guess it might be possible
> > > > > to reload the gfx driver. On a unpatched system the test that is
> > > > > included in the issue or the igt test that has been posted for the issue
> > > > > will show the problem.
> > > > > 
> > > > > I ran the test on an unpatched system here and the gui stopped and the
> > > > > keyboard stopped responding, so I rebooted. With the patched system I
> > > > > did not need to reboot.
> > > > > 
> > > > > Should I change the SIGTERM to SIGSEGV, not quite the same thing but
> > > > > tooling is better at handling a segfault than a SIGTERM and the
> > > > > application that calls this IOCTL is using an uninitialised hw lock so
> > > > > it is kind of the same as differencing an uninitialised pointer (kind
> > > > > of). Or, I could just remove it, but the bug has been in the code for at
> > > > > least two years (and known about), and I would guess that any code that
> > > > > is calling this is fuzzing the IOCTLs (as this is how the security team
> > > > > found it) and we should reward them with a application exit.
> > > > > 
> > > > > Peter. 
> > > > 
> > > > SIGSEGV would be a better choice.
> > > > 
> > > > SIGTERM is normally sent by a user -- it's the default signal sent by
> > > > kill(1). It's also commonly used to tell a long-running daemon process
> > > > to tidy up and exit cleanly.
> > > > 
> > > > SIGSEGV commonly means "you accessed something that doesn't exist/isn't
> > > > mapped/you don't have permissions for". There are specific subcases that
> > > > can be indicated via the siginfo data; this is from the sigaction(1)
> > > > manpage:
> > > > 
> > > >     The following values can be placed in si_code for a SIGSEGV signal:
> > > > 
> > > >         SEGV_MAPERR    address not mapped to object
> > > > 
> > > >         SEGV_ACCERR    invalid permissions for mapped object
> > > > 
> > > > SIGBUS would also be a possibility but that's generally taken to mean
> > > > that an access got all the way to some physical bus and then faulted,
> > > > whereas SIGSEGV suggests the access was rejected during the
> > > > virtual-to-physical mapping process.
> > > 
> > > None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.
> > 
> > Seconded, we really don't want to be in the business of fixing up the drm
> > design mistakes of the past 15 years. As long as we can fully lock out
> > this particular dragon when running i915 we're imo good enough. The dri1
> > design of a kernel shim driver cooperating with the ums driver for hw
> > ownership is fundamentally unfixable.
> > 
> > Also we can't change any of it for drivers actually using it since it'll
> > break them, which is a big no-go.
> > -Daniel
> 
> I will remove it. But, If you are using this code path the driver/kernel
> will have crashed. It covers a NULL pointer deference, so we are not
> changing the API that anyone is actually using.

With ums/dri1 userspace can crash the kernel. That's pretty much by
design, and it's pretty much unfixable. Trying to fix up the obvious ones
like here is just cargo-culting. The only real option is to make
absolutely sure we can't ever reach these codepaths at all from kms
drivers like i915. Which tends to be a lot more work for auditing, but
will be much more useful.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/5] drm: Possible lock priority escalation.

[Daniel Vetter]

On Tue, May 05, 2015 at 06:45:30AM +0000, Antoine, Peter wrote:
> On Mon, 2015-05-04 at 15:56 +0200, Daniel Vetter wrote:
> > On Mon, Apr 27, 2015 at 07:52:46PM +0300, Ville Syrjälä wrote:
> > > On Thu, Apr 23, 2015 at 03:07:56PM +0100, Peter Antoine wrote:
> > > > If an application that has a driver lock created, wants the lock the
> > > > kernel context, it is not allowed to. If the call to drm_lock has a
> > > > context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
> > > > then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
> > > > But as the DRM_LOCK_CONT bits are not part of the context id this allows
> > > > operations on the DRM_KERNEL_CONTEXT.
> > > > 
> > > > Issue: VIZ-5485
> > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > 
> > If you're touching code with drm_legacy_ prefix of in such a file you've
> > ended up in the horrible corners of the dri1 dungeons and should head back
> > out pronto ;-)
> > 
> > If we can actually run into this code on production i915 then we need to
> > improve the locks at the door of these dungeons for kms drivers, not try
> > to fix up the mess behind them. That's just plain impossible.
> > 
> > If you want to make really sure we get this right some simple drm igt
> > tests to make sure these codepaths are really dead for kms driver might be
> > good. But otherwise we really can only annotate this as wontfix in
> > code security issue scanners.
> > -Daniel
> > 
> There is a test that covers this fix. This is a simple three line fix
> that stops a userspace driver locking the kernel context. Yes they are
> other problems with this code, but why are they stopping this patch that
> does a simple fix from going in?
> 
> I'll happily drop this patch if it causes more problems that it fixes.

Because we don't want to fix the legacy context crap but instead outright
disable it for everyone (well except nouveau) running in kms code. drm
legacy code really is broken by design, there's no way to fix it.

And worst case we'll end up breaking some old machines in some and then
have to deal with the regression. Which means I won't apply these patches
if we can somehow just disable all that code for i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/2] HW_LOCK kernel patched.

[Peter Antoine]

This is a resubmission of the security fixed for a Kernel Crash found in the 
drm part of the driver. It was caused by a attempt to Lock/Unlock a HW_LOCK
that was uninitialised. As this feature is only required on some legacy drivers
this patchset now only turns these features off.

There is an igt test to follow, just working out a way of not crashing the
systems that have this feature-set turned on (probably need to create a context
and if that succeeds then exit as the feature is on.

V2: Remove the patches that cover the Kernel Crash and the Lock Escalation.
    Remove the PARAM that was used by the test to detect the presences of this
    feature.

Peter Antoine (2):
  drm: Make HW_LOCK access functions optional.
  drm: Make Legacy Context access functions optional.

 drivers/gpu/drm/drm_context.c         | 36 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c             | 12 +++++++-----
 drivers/gpu/drm/drm_lock.c            |  6 ++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
 include/drm/drmP.h                    | 23 +++++++++++-----------
 5 files changed, 63 insertions(+), 17 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm: Make HW_LOCK access functions optional.

[Peter Antoine]

As these functions are only used by one driver and there are security holes
in these functions. Make the functions optional.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_lock.c            |  6 ++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
 include/drm/drmP.h                    | 23 ++++++++++++-----------
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index f861361..21eb180 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
 	struct drm_master *master = file_priv->master;
 	int ret = 0;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	++file_priv->lock_count;
 
 	if (lock->context == DRM_KERNEL_CONTEXT) {
@@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
 	struct drm_lock *lock = data;
 	struct drm_master *master = file_priv->master;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	if (lock->context == DRM_KERNEL_CONTEXT) {
 		DRM_ERROR("Process %d using kernel context %d\n",
 			  task_pid_nr(current), lock->context);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 8904933..9624b38 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -941,7 +941,8 @@ static struct drm_driver
 driver_stub = {
 	.driver_features =
 		DRIVER_USE_AGP |
-		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
+		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
+		DRIVER_KMS_LEGACY_CONTEXT,
 
 	.load = nouveau_drm_load,
 	.unload = nouveau_drm_unload,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index df6d997..3874942 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -137,17 +137,18 @@ void drm_err(const char *format, ...);
 /*@{*/
 
 /* driver capabilities and requirements mask */
-#define DRIVER_USE_AGP     0x1
-#define DRIVER_PCI_DMA     0x8
-#define DRIVER_SG          0x10
-#define DRIVER_HAVE_DMA    0x20
-#define DRIVER_HAVE_IRQ    0x40
-#define DRIVER_IRQ_SHARED  0x80
-#define DRIVER_GEM         0x1000
-#define DRIVER_MODESET     0x2000
-#define DRIVER_PRIME       0x4000
-#define DRIVER_RENDER      0x8000
-#define DRIVER_ATOMIC      0x10000
+#define DRIVER_USE_AGP			0x1
+#define DRIVER_PCI_DMA			0x8
+#define DRIVER_SG			0x10
+#define DRIVER_HAVE_DMA			0x20
+#define DRIVER_HAVE_IRQ			0x40
+#define DRIVER_IRQ_SHARED		0x80
+#define DRIVER_GEM			0x1000
+#define DRIVER_MODESET			0x2000
+#define DRIVER_PRIME			0x4000
+#define DRIVER_RENDER			0x8000
+#define DRIVER_ATOMIC			0x10000
+#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
 
 /***********************************************************************/
 /** \name Macros to make printk easier */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/2] drm: Make Legacy Context access functions optional.

[Peter Antoine]

As these functions are only used by one driver and there are security holes
in these functions. Make the functions optional.

These changes are based on the two patches:
  commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
  Author: Dave Airlie <airlied@redhat.com>

And the commit that the above patch reverts:
  commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>

This should now turn off the context feature.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_context.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c     | 12 +++++++-----
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
index 9b23525..574be2a 100644
--- a/drivers/gpu/drm/drm_context.c
+++ b/drivers/gpu/drm/drm_context.c
@@ -53,6 +53,9 @@ struct drm_ctx_list {
  */
 void drm_legacy_ctxbitmap_free(struct drm_device * dev, int ctx_handle)
 {
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 	idr_remove(&dev->ctx_idr, ctx_handle);
 	mutex_unlock(&dev->struct_mutex);
@@ -87,6 +90,9 @@ static int drm_legacy_ctxbitmap_next(struct drm_device * dev)
  */
 int drm_legacy_ctxbitmap_init(struct drm_device * dev)
 {
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	idr_init(&dev->ctx_idr);
 	return 0;
 }
@@ -101,6 +107,9 @@ int drm_legacy_ctxbitmap_init(struct drm_device * dev)
  */
 void drm_legacy_ctxbitmap_cleanup(struct drm_device * dev)
 {
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 	idr_destroy(&dev->ctx_idr);
 	mutex_unlock(&dev->struct_mutex);
@@ -119,6 +128,9 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_ctx_list *pos, *tmp;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->ctxlist_mutex);
 
 	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
@@ -161,6 +173,9 @@ int drm_legacy_getsareactx(struct drm_device *dev, void *data,
 	struct drm_local_map *map;
 	struct drm_map_list *_entry;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 
 	map = idr_find(&dev->ctx_idr, request->ctx_id);
@@ -205,6 +220,9 @@ int drm_legacy_setsareactx(struct drm_device *dev, void *data,
 	struct drm_local_map *map = NULL;
 	struct drm_map_list *r_list = NULL;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	mutex_lock(&dev->struct_mutex);
 	list_for_each_entry(r_list, &dev->maplist, head) {
 		if (r_list->map
@@ -305,6 +323,9 @@ int drm_legacy_resctx(struct drm_device *dev, void *data,
 	struct drm_ctx ctx;
 	int i;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	if (res->count >= DRM_RESERVED_CONTEXTS) {
 		memset(&ctx, 0, sizeof(ctx));
 		for (i = 0; i < DRM_RESERVED_CONTEXTS; i++) {
@@ -335,6 +356,9 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
 	struct drm_ctx_list *ctx_entry;
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	ctx->handle = drm_legacy_ctxbitmap_next(dev);
 	if (ctx->handle == DRM_KERNEL_CONTEXT) {
 		/* Skip kernel's context and get a new one. */
@@ -378,6 +402,9 @@ int drm_legacy_getctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	/* This is 0, because we don't handle any context flags */
 	ctx->flags = 0;
 
@@ -400,6 +427,9 @@ int drm_legacy_switchctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	DRM_DEBUG("%d\n", ctx->handle);
 	return drm_context_switch(dev, dev->last_context, ctx->handle);
 }
@@ -420,6 +450,9 @@ int drm_legacy_newctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	DRM_DEBUG("%d\n", ctx->handle);
 	drm_context_switch_complete(dev, file_priv, ctx->handle);
 
@@ -442,6 +475,9 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
 {
 	struct drm_ctx *ctx = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	DRM_DEBUG("%d\n", ctx->handle);
 	if (ctx->handle != DRM_KERNEL_CONTEXT) {
 		if (dev->driver->context_dtor)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3b3c4f5..1de2df8 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -584,11 +584,13 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 	if (drm_ht_create(&dev->map_hash, 12))
 		goto err_minors;
 
-	ret = drm_legacy_ctxbitmap_init(dev);
-	if (ret) {
-		DRM_ERROR("Cannot allocate memory for context bitmap.\n");
-		goto err_ht;
-	}
+	if (drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		ret = drm_legacy_ctxbitmap_init(dev);
+		if (ret) {
+			DRM_ERROR(
+				"Cannot allocate memory for context bitmap.\n");
+			goto err_ht;
+		}
 
 	if (drm_core_check_feature(dev, DRIVER_GEM)) {
 		ret = drm_gem_init(dev);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 0/2] HW_LOCK kernel patched.

[Daniel Vetter]

On Wed, May 13, 2015 at 07:54:46AM +0100, Peter Antoine wrote:
> This is a resubmission of the security fixed for a Kernel Crash found in the 
> drm part of the driver. It was caused by a attempt to Lock/Unlock a HW_LOCK
> that was uninitialised. As this feature is only required on some legacy drivers
> this patchset now only turns these features off.
> 
> There is an igt test to follow, just working out a way of not crashing the
> systems that have this feature-set turned on (probably need to create a context
> and if that succeeds then exit as the feature is on.
> 
> V2: Remove the patches that cover the Kernel Crash and the Lock Escalation.
>     Remove the PARAM that was used by the test to detect the presences of this
>     feature.

Process nit: When resending the entire patch series anew please start a
new thread. That avoids confusion with the discussion on the old patches
(I've managed to wreak havoc and merge a mix of old&new patches this way
already). in-reply-to resending is just for resending parts of the series
when you do a quick update of a patch.
-Daniel

> 
> Peter Antoine (2):
>   drm: Make HW_LOCK access functions optional.
>   drm: Make Legacy Context access functions optional.
> 
>  drivers/gpu/drm/drm_context.c         | 36 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_drv.c             | 12 +++++++-----
>  drivers/gpu/drm/drm_lock.c            |  6 ++++++
>  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
>  include/drm/drmP.h                    | 23 +++++++++++-----------
>  5 files changed, 63 insertions(+), 17 deletions(-)
> 
> -- 
> 1.9.1
> 

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

Re: [PATCH v2 1/2] drm: Make HW_LOCK access functions optional.

[Daniel Vetter]

On Wed, May 13, 2015 at 07:54:47AM +0100, Peter Antoine wrote:
> As these functions are only used by one driver and there are security holes
> in these functions. Make the functions optional.

Is there a reference for why nouveau needs hw locks too? Also have you
done an audit of mesa history and X history to make sure there's no other
driver accidentally using it with a modern kms driver?

> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/drm_lock.c            |  6 ++++++
>  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
>  include/drm/drmP.h                    | 23 ++++++++++++-----------
>  3 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index f861361..21eb180 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>  	struct drm_master *master = file_priv->master;
>  	int ret = 0;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))

You also need to allow these for all legacy drivers, i.e. without
DRIVER_MODESET.
-Daniel

> +		return -EINVAL;
> +
>  	++file_priv->lock_count;
>  
>  	if (lock->context == DRM_KERNEL_CONTEXT) {
> @@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
>  	struct drm_lock *lock = data;
>  	struct drm_master *master = file_priv->master;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	if (lock->context == DRM_KERNEL_CONTEXT) {
>  		DRM_ERROR("Process %d using kernel context %d\n",
>  			  task_pid_nr(current), lock->context);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 8904933..9624b38 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -941,7 +941,8 @@ static struct drm_driver
>  driver_stub = {
>  	.driver_features =
>  		DRIVER_USE_AGP |
> -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> +		DRIVER_KMS_LEGACY_CONTEXT,
>  
>  	.load = nouveau_drm_load,
>  	.unload = nouveau_drm_unload,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index df6d997..3874942 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);
>  /*@{*/
>  
>  /* driver capabilities and requirements mask */
> -#define DRIVER_USE_AGP     0x1
> -#define DRIVER_PCI_DMA     0x8
> -#define DRIVER_SG          0x10
> -#define DRIVER_HAVE_DMA    0x20
> -#define DRIVER_HAVE_IRQ    0x40
> -#define DRIVER_IRQ_SHARED  0x80
> -#define DRIVER_GEM         0x1000
> -#define DRIVER_MODESET     0x2000
> -#define DRIVER_PRIME       0x4000
> -#define DRIVER_RENDER      0x8000
> -#define DRIVER_ATOMIC      0x10000
> +#define DRIVER_USE_AGP			0x1
> +#define DRIVER_PCI_DMA			0x8
> +#define DRIVER_SG			0x10
> +#define DRIVER_HAVE_DMA			0x20
> +#define DRIVER_HAVE_IRQ			0x40
> +#define DRIVER_IRQ_SHARED		0x80
> +#define DRIVER_GEM			0x1000
> +#define DRIVER_MODESET			0x2000
> +#define DRIVER_PRIME			0x4000
> +#define DRIVER_RENDER			0x8000
> +#define DRIVER_ATOMIC			0x10000
> +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
>  
>  /***********************************************************************/
>  /** \name Macros to make printk easier */
> -- 
> 1.9.1
> 

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

Re: [PATCH v2 2/2] drm: Make Legacy Context access functions optional.

[Daniel Vetter]

On Wed, May 13, 2015 at 07:54:48AM +0100, Peter Antoine wrote:
> As these functions are only used by one driver and there are security holes
> in these functions. Make the functions optional.
> 
> These changes are based on the two patches:
>   commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
>   Author: Dave Airlie <airlied@redhat.com>
> 
> And the commit that the above patch reverts:
>   commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> This should now turn off the context feature.
> 
> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>

Please cc drm core patches to dri-devel. Review below.
-Daniel

> ---
>  drivers/gpu/drm/drm_context.c | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_drv.c     | 12 +++++++-----
>  2 files changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> index 9b23525..574be2a 100644
> --- a/drivers/gpu/drm/drm_context.c
> +++ b/drivers/gpu/drm/drm_context.c
> @@ -53,6 +53,9 @@ struct drm_ctx_list {
>   */
>  void drm_legacy_ctxbitmap_free(struct drm_device * dev, int ctx_handle)
>  {
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;

This doesn't compile too well. Also like with the previous patch, drivers
without DRIVER_MODESET still need these ioctls.

> +
>  	mutex_lock(&dev->struct_mutex);
>  	idr_remove(&dev->ctx_idr, ctx_handle);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -87,6 +90,9 @@ static int drm_legacy_ctxbitmap_next(struct drm_device * dev)
>   */
>  int drm_legacy_ctxbitmap_init(struct drm_device * dev)
>  {
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;

This is redundant with the check you've added around the caller. Looking
at other places wrapping callers of _legacy_ functions with these checks
is the more common pattern in drm.

> +
>  	idr_init(&dev->ctx_idr);
>  	return 0;
>  }
> @@ -101,6 +107,9 @@ int drm_legacy_ctxbitmap_init(struct drm_device * dev)
>   */
>  void drm_legacy_ctxbitmap_cleanup(struct drm_device * dev)
>  {
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;

Same issue with compiling. And probably better to move the check out into
callers.

> +
>  	mutex_lock(&dev->struct_mutex);
>  	idr_destroy(&dev->ctx_idr);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -119,6 +128,9 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct drm_ctx_list *pos, *tmp;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;

Same as above.

> +
>  	mutex_lock(&dev->ctxlist_mutex);
>  
>  	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
> @@ -161,6 +173,9 @@ int drm_legacy_getsareactx(struct drm_device *dev, void *data,
>  	struct drm_local_map *map;
>  	struct drm_map_list *_entry;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	mutex_lock(&dev->struct_mutex);
>  
>  	map = idr_find(&dev->ctx_idr, request->ctx_id);
> @@ -205,6 +220,9 @@ int drm_legacy_setsareactx(struct drm_device *dev, void *data,
>  	struct drm_local_map *map = NULL;
>  	struct drm_map_list *r_list = NULL;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	mutex_lock(&dev->struct_mutex);
>  	list_for_each_entry(r_list, &dev->maplist, head) {
>  		if (r_list->map
> @@ -305,6 +323,9 @@ int drm_legacy_resctx(struct drm_device *dev, void *data,
>  	struct drm_ctx ctx;
>  	int i;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	if (res->count >= DRM_RESERVED_CONTEXTS) {
>  		memset(&ctx, 0, sizeof(ctx));
>  		for (i = 0; i < DRM_RESERVED_CONTEXTS; i++) {
> @@ -335,6 +356,9 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
>  	struct drm_ctx_list *ctx_entry;
>  	struct drm_ctx *ctx = data;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	ctx->handle = drm_legacy_ctxbitmap_next(dev);
>  	if (ctx->handle == DRM_KERNEL_CONTEXT) {
>  		/* Skip kernel's context and get a new one. */
> @@ -378,6 +402,9 @@ int drm_legacy_getctx(struct drm_device *dev, void *data,
>  {
>  	struct drm_ctx *ctx = data;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	/* This is 0, because we don't handle any context flags */
>  	ctx->flags = 0;
>  
> @@ -400,6 +427,9 @@ int drm_legacy_switchctx(struct drm_device *dev, void *data,
>  {
>  	struct drm_ctx *ctx = data;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	DRM_DEBUG("%d\n", ctx->handle);
>  	return drm_context_switch(dev, dev->last_context, ctx->handle);
>  }
> @@ -420,6 +450,9 @@ int drm_legacy_newctx(struct drm_device *dev, void *data,
>  {
>  	struct drm_ctx *ctx = data;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	DRM_DEBUG("%d\n", ctx->handle);
>  	drm_context_switch_complete(dev, file_priv, ctx->handle);
>  
> @@ -442,6 +475,9 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
>  {
>  	struct drm_ctx *ctx = data;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	DRM_DEBUG("%d\n", ctx->handle);
>  	if (ctx->handle != DRM_KERNEL_CONTEXT) {
>  		if (dev->driver->context_dtor)
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 3b3c4f5..1de2df8 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -584,11 +584,13 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>  	if (drm_ht_create(&dev->map_hash, 12))
>  		goto err_minors;
>  
> -	ret = drm_legacy_ctxbitmap_init(dev);
> -	if (ret) {
> -		DRM_ERROR("Cannot allocate memory for context bitmap.\n");
> -		goto err_ht;
> -	}
> +	if (drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))

Coding style requires {} since it's multi-line. And confusing since the if
(ret) gets always executed.

> +		ret = drm_legacy_ctxbitmap_init(dev);
> +		if (ret) {
> +			DRM_ERROR(
> +				"Cannot allocate memory for context bitmap.\n");
> +			goto err_ht;
> +		}
>  
>  	if (drm_core_check_feature(dev, DRIVER_GEM)) {
>  		ret = drm_gem_init(dev);
> -- 
> 1.9.1
> 

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

Re: [PATCH v2 1/2] drm: Make HW_LOCK access functions optional.

[Daniel Vetter]

Adding dri-devel, I've forgotten to do that ...
-Daniel

On Wed, May 13, 2015 at 09:14:29AM +0200, Daniel Vetter wrote:
> On Wed, May 13, 2015 at 07:54:47AM +0100, Peter Antoine wrote:
> > As these functions are only used by one driver and there are security holes
> > in these functions. Make the functions optional.
> 
> Is there a reference for why nouveau needs hw locks too? Also have you
> done an audit of mesa history and X history to make sure there's no other
> driver accidentally using it with a modern kms driver?
> 
> > Issue: VIZ-5485
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > ---
> >  drivers/gpu/drm/drm_lock.c            |  6 ++++++
> >  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
> >  include/drm/drmP.h                    | 23 ++++++++++++-----------
> >  3 files changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > index f861361..21eb180 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> >  	struct drm_master *master = file_priv->master;
> >  	int ret = 0;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> 
> You also need to allow these for all legacy drivers, i.e. without
> DRIVER_MODESET.
> -Daniel
> 
> > +		return -EINVAL;
> > +
> >  	++file_priv->lock_count;
> >  
> >  	if (lock->context == DRM_KERNEL_CONTEXT) {
> > @@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> >  	struct drm_lock *lock = data;
> >  	struct drm_master *master = file_priv->master;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > +		return -EINVAL;
> > +
> >  	if (lock->context == DRM_KERNEL_CONTEXT) {
> >  		DRM_ERROR("Process %d using kernel context %d\n",
> >  			  task_pid_nr(current), lock->context);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 8904933..9624b38 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -941,7 +941,8 @@ static struct drm_driver
> >  driver_stub = {
> >  	.driver_features =
> >  		DRIVER_USE_AGP |
> > -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> > +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> > +		DRIVER_KMS_LEGACY_CONTEXT,
> >  
> >  	.load = nouveau_drm_load,
> >  	.unload = nouveau_drm_unload,
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index df6d997..3874942 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);
> >  /*@{*/
> >  
> >  /* driver capabilities and requirements mask */
> > -#define DRIVER_USE_AGP     0x1
> > -#define DRIVER_PCI_DMA     0x8
> > -#define DRIVER_SG          0x10
> > -#define DRIVER_HAVE_DMA    0x20
> > -#define DRIVER_HAVE_IRQ    0x40
> > -#define DRIVER_IRQ_SHARED  0x80
> > -#define DRIVER_GEM         0x1000
> > -#define DRIVER_MODESET     0x2000
> > -#define DRIVER_PRIME       0x4000
> > -#define DRIVER_RENDER      0x8000
> > -#define DRIVER_ATOMIC      0x10000
> > +#define DRIVER_USE_AGP			0x1
> > +#define DRIVER_PCI_DMA			0x8
> > +#define DRIVER_SG			0x10
> > +#define DRIVER_HAVE_DMA			0x20
> > +#define DRIVER_HAVE_IRQ			0x40
> > +#define DRIVER_IRQ_SHARED		0x80
> > +#define DRIVER_GEM			0x1000
> > +#define DRIVER_MODESET			0x2000
> > +#define DRIVER_PRIME			0x4000
> > +#define DRIVER_RENDER			0x8000
> > +#define DRIVER_ATOMIC			0x10000
> > +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> >  
> >  /***********************************************************************/
> >  /** \name Macros to make printk easier */
> > -- 
> > 1.9.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v2 2/2] drm: Make Legacy Context access functions optional.

[Ville Syrjälä]

On Wed, May 13, 2015 at 09:19:09AM +0200, Daniel Vetter wrote:
> On Wed, May 13, 2015 at 07:54:48AM +0100, Peter Antoine wrote:
> > As these functions are only used by one driver and there are security holes
> > in these functions. Make the functions optional.
> > 
> > These changes are based on the two patches:
> >   commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
> >   Author: Dave Airlie <airlied@redhat.com>
> > 
> > And the commit that the above patch reverts:
> >   commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
> >   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > This should now turn off the context feature.
> > 
> > Issue: VIZ-5485
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> 
> Please cc drm core patches to dri-devel. Review below.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_context.c | 36 ++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_drv.c     | 12 +++++++-----
> >  2 files changed, 43 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> > index 9b23525..574be2a 100644
> > --- a/drivers/gpu/drm/drm_context.c
> > +++ b/drivers/gpu/drm/drm_context.c
> > @@ -53,6 +53,9 @@ struct drm_ctx_list {
> >   */
> >  void drm_legacy_ctxbitmap_free(struct drm_device * dev, int ctx_handle)
> >  {
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > +		return -EINVAL;
> 
> This doesn't compile too well. Also like with the previous patch, drivers
> without DRIVER_MODESET still need these ioctls.

Only via actually needs them AFAIK, but continuing to allow them for the
rest of the old drivers is no worse than what we have today.

And I think nouveau just needs a nop create/destroy ioctls, so I think
we should really just make those two nops for nouveau (or maybe for all
modern drivers?) and reject the rest of the ioctls even for nouveau.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: Make Legacy Context access functions optional.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6394
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              272/272              270/272
ILK                                  302/302              302/302
SNB                 -1              315/315              314/315
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                                  317/317              317/317
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt@gem_tiled_pread_pwrite      PASS(4)      FAIL(1)
*PNV  igt@gem_userptr_blits@coherency-sync      PASS(4)      CRASH(1)
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(7)PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx