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

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