[v2,2/2] drm: Make Legacy Context access functions optional.
diff mbox

Message ID 1431500088-15278-3-git-send-email-peter.antoine@intel.com
State New
Headers show

Commit Message

Peter Antoine May 13, 2015, 6:54 a.m. UTC
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(-)

Comments

Daniel Vetter May 13, 2015, 7:19 a.m. UTC | #1
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
>
Ville Syrjälä May 13, 2015, 9:41 a.m. UTC | #2
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.
Shuang He May 15, 2015, 5:58 a.m. UTC | #3
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 '*'

Patch
diff mbox

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