From 09f308f7b675b692d1c0a451ff02f0fff7846c96 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 13 Mar 2014 10:00:42 +0100 Subject: [PATCH 01/16] drm: Have the crtc code only reference master from legacy nodes v2 control- and render nodes are intended to be master-less. v2: Replace tests for !legacy with tests for !mode_group for readability. Signed-off-by: Thomas Hellstrom Reviewed-by: David Herrmann --- drivers/gpu/drm/drm_crtc.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 16ca28ed5ee8e..5fb02d57dae41 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1492,9 +1492,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data, mutex_unlock(&file_priv->fbs_lock); drm_modeset_lock_all(dev); - mode_group = &file_priv->master->minor->mode_group; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (file_priv->minor->type != DRM_MINOR_LEGACY) { + mode_group = NULL; list_for_each(lh, &dev->mode_config.crtc_list) crtc_count++; @@ -1505,6 +1505,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, encoder_count++; } else { + mode_group = &file_priv->master->minor->mode_group; crtc_count = mode_group->num_crtcs; connector_count = mode_group->num_connectors; encoder_count = mode_group->num_encoders; @@ -1519,7 +1520,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, if (card_res->count_crtcs >= crtc_count) { copied = 0; crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (!mode_group) { list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id); @@ -1546,7 +1547,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, if (card_res->count_encoders >= encoder_count) { copied = 0; encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (!mode_group) { list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { @@ -1577,7 +1578,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, if (card_res->count_connectors >= connector_count) { copied = 0; connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; - if (file_priv->master->minor->type == DRM_MINOR_CONTROL) { + if (!mode_group) { list_for_each_entry(connector, &dev->mode_config.connector_list, head) { @@ -2846,7 +2847,8 @@ int drm_mode_getfb(struct drm_device *dev, r->bpp = fb->bits_per_pixel; r->pitch = fb->pitches[0]; if (fb->funcs->create_handle) { - if (file_priv->is_master || capable(CAP_SYS_ADMIN)) { + if (file_priv->is_master || capable(CAP_SYS_ADMIN) || + file_priv->minor->type == DRM_MINOR_CONTROL) { ret = fb->funcs->create_handle(fb, file_priv, &r->handle); } else { From c2667355619572c9324916b62b4f6608a56de031 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 13 Mar 2014 10:30:25 +0100 Subject: [PATCH 02/16] drm: Break out ioctl permission check to a separate function v2 Helps reviewing and understanding these checks. v2: Remove misplaced newlines. Signed-off-by: Thomas Hellstrom Reviewed-by: David Herrmann --- drivers/gpu/drm/drm_drv.c | 113 +++++++++++++++++++++++++------------- 1 file changed, 75 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index ec651be2f3cbd..05e30530c025c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -285,6 +285,44 @@ static int drm_version(struct drm_device *dev, void *data, return err; } +/** + * drm_ioctl_permit - Check ioctl permissions against caller + * + * @flags: ioctl permission flags. + * @file_priv: Pointer to struct drm_file identifying the caller. + * + * Checks whether the caller is allowed to run an ioctl with the + * indicated permissions. If so, returns zero. Otherwise returns an + * error code suitable for ioctl return. + */ +static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) +{ + /* ROOT_ONLY is only for CAP_SYS_ADMIN */ + if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN))) + return -EACCES; + + /* AUTH is only for authenticated or render client */ + if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && + !file_priv->authenticated)) + return -EACCES; + + /* MASTER is only for master */ + if (unlikely((flags & DRM_MASTER) && !file_priv->is_master)) + return -EACCES; + + /* Control clients must be explicitly allowed */ + if (unlikely(!(flags & DRM_CONTROL_ALLOW) && + file_priv->minor->type == DRM_MINOR_CONTROL)) + return -EACCES; + + /* Render clients must be explicitly allowed */ + if (unlikely(!(flags & DRM_RENDER_ALLOW) && + drm_is_render_client(file_priv))) + return -EACCES; + + return 0; +} + /** * Called whenever a process performs an ioctl on /dev/drm. * @@ -350,52 +388,51 @@ long drm_ioctl(struct file *filp, /* Do not trust userspace, use our own definition */ func = ioctl->func; - if (!func) { + if (unlikely(!func)) { DRM_DEBUG("no function\n"); retcode = -EINVAL; - } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || - ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || - ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || - (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) { - retcode = -EACCES; - } else { - if (cmd & (IOC_IN | IOC_OUT)) { - if (asize <= sizeof(stack_kdata)) { - kdata = stack_kdata; - } else { - kdata = kmalloc(asize, GFP_KERNEL); - if (!kdata) { - retcode = -ENOMEM; - goto err_i1; - } - } - if (asize > usize) - memset(kdata + usize, 0, asize - usize); - } + goto err_i1; + } - if (cmd & IOC_IN) { - if (copy_from_user(kdata, (void __user *)arg, - usize) != 0) { - retcode = -EFAULT; + retcode = drm_ioctl_permit(ioctl->flags, file_priv); + if (unlikely(retcode)) + goto err_i1; + + if (cmd & (IOC_IN | IOC_OUT)) { + if (asize <= sizeof(stack_kdata)) { + kdata = stack_kdata; + } else { + kdata = kmalloc(asize, GFP_KERNEL); + if (!kdata) { + retcode = -ENOMEM; goto err_i1; } - } else - memset(kdata, 0, usize); - - if (ioctl->flags & DRM_UNLOCKED) - retcode = func(dev, kdata, file_priv); - else { - mutex_lock(&drm_global_mutex); - retcode = func(dev, kdata, file_priv); - mutex_unlock(&drm_global_mutex); } + if (asize > usize) + memset(kdata + usize, 0, asize - usize); + } - if (cmd & IOC_OUT) { - if (copy_to_user((void __user *)arg, kdata, - usize) != 0) - retcode = -EFAULT; + if (cmd & IOC_IN) { + if (copy_from_user(kdata, (void __user *)arg, + usize) != 0) { + retcode = -EFAULT; + goto err_i1; } + } else + memset(kdata, 0, usize); + + if (ioctl->flags & DRM_UNLOCKED) + retcode = func(dev, kdata, file_priv); + else { + mutex_lock(&drm_global_mutex); + retcode = func(dev, kdata, file_priv); + mutex_unlock(&drm_global_mutex); + } + + if (cmd & IOC_OUT) { + if (copy_to_user((void __user *)arg, kdata, + usize) != 0) + retcode = -EFAULT; } err_i1: From ac05dbc57ef2b8709bf48693bb25e16a63e8e71f Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Wed, 19 Feb 2014 14:21:48 +0100 Subject: [PATCH 03/16] drm: Make control nodes master-less v3 Like for render-nodes, there is no point in maintaining the master concept for control nodes, so set the struct drm_file::master pointer to NULL. At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always allowed when called through the control node. Previously the caller also needed to be master. v2: Adapt to refactoring of ioctl permission check. v3: Formatting of logical expression. Use drm_is_control_client() instead of drm_is_control(). Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul Reviewed-by: David Herrmann --- drivers/gpu/drm/drm_drv.c | 7 ++++--- drivers/gpu/drm/drm_fops.c | 6 ++++-- include/drm/drmP.h | 5 +++++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 05e30530c025c..cf2dfb790bf1e 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -306,13 +306,14 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) !file_priv->authenticated)) return -EACCES; - /* MASTER is only for master */ - if (unlikely((flags & DRM_MASTER) && !file_priv->is_master)) + /* MASTER is only for master or control clients */ + if (unlikely((flags & DRM_MASTER) && !file_priv->is_master && + !drm_is_control_client(file_priv))) return -EACCES; /* Control clients must be explicitly allowed */ if (unlikely(!(flags & DRM_CONTROL_ALLOW) && - file_priv->minor->type == DRM_MINOR_CONTROL)) + drm_is_control_client(file_priv))) return -EACCES; /* Render clients must be explicitly allowed */ diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 9b02f126fb0d9..5432a1a61c153 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -232,7 +232,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* if there is no current master make this fd it, but do not create * any master object for render clients */ mutex_lock(&dev->struct_mutex); - if (!priv->minor->master && !drm_is_render_client(priv)) { + if (!priv->minor->master && !drm_is_render_client(priv) && + !drm_is_control_client(priv)) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -270,7 +271,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, goto out_close; } } - } else if (!drm_is_render_client(priv)) { + } else if (!drm_is_render_client(priv) && + !drm_is_control_client(priv)) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2242968e7deb7..3cf9f46ce2e6e 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1207,6 +1207,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_RENDER; } +static inline bool drm_is_control_client(const struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_CONTROL; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/ From 436830571eb9045d563979dc6185b1d5145ca4b6 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 13 Mar 2014 11:07:44 +0100 Subject: [PATCH 04/16] drm: Improve on minor type helpers v3 Add a drm_is_legacy() helper, constify argument to drm_is_render_client(), and use / change helpers where appropriate. v2: s/drm_is_legacy/drm_is_legacy_client/ and adapt to new code context. v3: s/legacy_client/primary_client/ Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/drm_crtc.c | 4 ++-- drivers/gpu/drm/drm_fops.c | 6 ++---- include/drm/drmP.h | 7 ++++++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5fb02d57dae41..960ca987c20fc 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1492,7 +1492,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, mutex_unlock(&file_priv->fbs_lock); drm_modeset_lock_all(dev); - if (file_priv->minor->type != DRM_MINOR_LEGACY) { + if (!drm_is_primary_client(file_priv)) { mode_group = NULL; list_for_each(lh, &dev->mode_config.crtc_list) @@ -2848,7 +2848,7 @@ int drm_mode_getfb(struct drm_device *dev, r->pitch = fb->pitches[0]; if (fb->funcs->create_handle) { if (file_priv->is_master || capable(CAP_SYS_ADMIN) || - file_priv->minor->type == DRM_MINOR_CONTROL) { + drm_is_control_client(file_priv)) { ret = fb->funcs->create_handle(fb, file_priv, &r->handle); } else { diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 5432a1a61c153..c7792b1d17739 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -232,8 +232,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* if there is no current master make this fd it, but do not create * any master object for render clients */ mutex_lock(&dev->struct_mutex); - if (!priv->minor->master && !drm_is_render_client(priv) && - !drm_is_control_client(priv)) { + if (drm_is_primary_client(priv) && !priv->minor->master) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -271,8 +270,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, goto out_close; } } - } else if (!drm_is_render_client(priv) && - !drm_is_control_client(priv)) { + } else if (drm_is_primary_client(priv)) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3cf9f46ce2e6e..3d06f71bc691f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1202,7 +1202,7 @@ static inline bool drm_modeset_is_locked(struct drm_device *dev) return mutex_is_locked(&dev->mode_config.mutex); } -static inline bool drm_is_render_client(struct drm_file *file_priv) +static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; } @@ -1212,6 +1212,11 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_CONTROL; } +static inline bool drm_is_primary_client(const struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_LEGACY; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/ From a12cd0025cdc0b4d43b79249dbd8c266af284032 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Wed, 19 Feb 2014 14:37:44 +0100 Subject: [PATCH 05/16] drm: Remove the minor master list It doesn't appear to be used anywhere. Signed-off-by: Thomas Hellstrom Reviewed-by: David Herrmann --- drivers/gpu/drm/drm_stub.c | 5 ----- include/drm/drmP.h | 2 -- 2 files changed, 7 deletions(-) diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index ed8a99576df3b..a378af49ed2e6 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -127,8 +127,6 @@ struct drm_master *drm_master_create(struct drm_minor *minor) INIT_LIST_HEAD(&master->magicfree); master->minor = minor; - list_add_tail(&master->head, &minor->master_list); - return master; } @@ -146,8 +144,6 @@ static void drm_master_destroy(struct kref *kref) struct drm_device *dev = master->minor->dev; struct drm_map_list *r_list, *list_temp; - list_del(&master->head); - if (dev->driver->master_destroy) dev->driver->master_destroy(dev, master); @@ -273,7 +269,6 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) minor->type = type; minor->dev = dev; - INIT_LIST_HEAD(&minor->master_list); *drm_minor_get_slot(dev, type) = minor; return 0; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3d06f71bc691f..3d594ca7fa62b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -689,7 +689,6 @@ struct drm_master { struct kref refcount; /* refcount for this master */ - struct list_head head; /**< each minor contains a list of masters */ struct drm_minor *minor; /**< link back to minor we are a master for */ char *unique; /**< Unique identifier: e.g., busid */ @@ -1022,7 +1021,6 @@ struct drm_minor { struct mutex debugfs_lock; /* Protects debugfs_list. */ struct drm_master *master; /* currently active master for this node */ - struct list_head master_list; struct drm_mode_group mode_group; }; From c996fd0b956450563454e7ccc97a82ca31f9d043 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Tue, 25 Feb 2014 19:57:44 +0100 Subject: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v3 The master management was previously protected by the drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separate master_mutex. Locking order is master_mutex -> struct_mutex. Also remove drm_master::blocked since it's not used. v2: Add an inline comment about what drm_device::master_mutex is protecting. v3: Remove unneeded struct_mutex locks. Fix error returns in drm_setmaster_ioctl(). Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul Reviewed-by: David Herrmann Acked-by: Daniel Vetter --- drivers/gpu/drm/drm_fops.c | 22 +++++++++--------- drivers/gpu/drm/drm_stub.c | 43 ++++++++++++++++++++++------------- include/drm/drmP.h | 46 +++++++++++++++++++++----------------- 3 files changed, 64 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1d17739..a0ce39c96f8e9 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -231,12 +231,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* if there is no current master make this fd it, but do not create * any master object for render clients */ - mutex_lock(&dev->struct_mutex); + mutex_lock(&dev->master_mutex); if (drm_is_primary_client(priv) && !priv->minor->master) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { - mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; goto out_close; } @@ -244,29 +243,23 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->is_master = 1; /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master); - priv->authenticated = 1; - mutex_unlock(&dev->struct_mutex); if (dev->driver->master_create) { ret = dev->driver->master_create(dev, priv->master); if (ret) { - mutex_lock(&dev->struct_mutex); /* drop both references if this fails */ drm_master_put(&priv->minor->master); drm_master_put(&priv->master); - mutex_unlock(&dev->struct_mutex); goto out_close; } } - mutex_lock(&dev->struct_mutex); if (dev->driver->master_set) { ret = dev->driver->master_set(dev, priv, true); if (ret) { /* drop both references if this fails */ drm_master_put(&priv->minor->master); drm_master_put(&priv->master); - mutex_unlock(&dev->struct_mutex); goto out_close; } } @@ -274,7 +267,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&dev->master_mutex); mutex_lock(&dev->struct_mutex); list_add(&priv->lhead, &dev->filelist); @@ -302,6 +295,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, return 0; out_close: + mutex_unlock(&dev->master_mutex); if (dev->driver->postclose) dev->driver->postclose(dev, priv); out_prime_destroy: @@ -489,11 +483,13 @@ int drm_release(struct inode *inode, struct file *filp) } mutex_unlock(&dev->ctxlist_mutex); - mutex_lock(&dev->struct_mutex); + mutex_lock(&dev->master_mutex); if (file_priv->is_master) { struct drm_master *master = file_priv->master; struct drm_file *temp; + + mutex_lock(&dev->struct_mutex); list_for_each_entry(temp, &dev->filelist, lhead) { if ((temp->master == file_priv->master) && (temp != file_priv)) @@ -512,6 +508,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv = NULL; wake_up_interruptible_all(&master->lock.lock_queue); } + mutex_unlock(&dev->struct_mutex); if (file_priv->minor->master == file_priv->master) { /* drop the reference held my the minor */ @@ -521,10 +518,13 @@ int drm_release(struct inode *inode, struct file *filp) } } - /* drop the reference held my the file priv */ + /* drop the master reference held by the file priv */ if (file_priv->master) drm_master_put(&file_priv->master); file_priv->is_master = 0; + mutex_unlock(&dev->master_mutex); + + mutex_lock(&dev->struct_mutex); list_del(&file_priv->lhead); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index a378af49ed2e6..fac6f98342577 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -144,6 +144,7 @@ static void drm_master_destroy(struct kref *kref) struct drm_device *dev = master->minor->dev; struct drm_map_list *r_list, *list_temp; + mutex_lock(&dev->struct_mutex); if (dev->driver->master_destroy) dev->driver->master_destroy(dev, master); @@ -171,6 +172,7 @@ static void drm_master_destroy(struct kref *kref) drm_ht_remove(&master->magiclist); + mutex_unlock(&dev->struct_mutex); kfree(master); } @@ -186,19 +188,20 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, { int ret = 0; + mutex_lock(&dev->master_mutex); if (file_priv->is_master) - return 0; + goto out_unlock; - if (file_priv->minor->master && file_priv->minor->master != file_priv->master) - return -EINVAL; - - if (!file_priv->master) - return -EINVAL; + if (file_priv->minor->master) { + ret = -EINVAL; + goto out_unlock; + } - if (file_priv->minor->master) - return -EINVAL; + if (!file_priv->master) { + ret = -EINVAL; + goto out_unlock; + } - mutex_lock(&dev->struct_mutex); file_priv->minor->master = drm_master_get(file_priv->master); file_priv->is_master = 1; if (dev->driver->master_set) { @@ -208,27 +211,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, drm_master_put(&file_priv->minor->master); } } - mutex_unlock(&dev->struct_mutex); +out_unlock: + mutex_unlock(&dev->master_mutex); return ret; } int drm_dropmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + int ret = -EINVAL; + + mutex_lock(&dev->master_mutex); if (!file_priv->is_master) - return -EINVAL; + goto out_unlock; if (!file_priv->minor->master) - return -EINVAL; + goto out_unlock; - mutex_lock(&dev->struct_mutex); + ret = 0; if (dev->driver->master_drop) dev->driver->master_drop(dev, file_priv, false); drm_master_put(&file_priv->minor->master); file_priv->is_master = 0; - mutex_unlock(&dev->struct_mutex); - return 0; + +out_unlock: + mutex_unlock(&dev->master_mutex); + return ret; } /* @@ -559,6 +568,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->ctxlist_mutex); + mutex_init(&dev->master_mutex); dev->anon_inode = drm_fs_inode_new(); if (IS_ERR(dev->anon_inode)) { @@ -612,6 +622,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode); err_free: + mutex_destroy(&dev->master_mutex); kfree(dev); return NULL; } @@ -633,6 +644,8 @@ static void drm_dev_release(struct kref *ref) drm_minor_free(dev, DRM_MINOR_CONTROL); kfree(dev->devname); + + mutex_destroy(&dev->master_mutex); kfree(dev); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3d594ca7fa62b..4e24a1a0daeb0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -405,7 +405,8 @@ struct drm_prime_file_private { struct drm_file { unsigned always_authenticated :1; unsigned authenticated :1; - unsigned is_master :1; /* this file private is a master for a minor */ + /* Whether we're master for a minor. Protected by master_mutex */ + unsigned is_master :1; /* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1; @@ -684,28 +685,29 @@ struct drm_gem_object { #include -/* per-master structure */ +/** + * struct drm_master - drm master structure + * + * @refcount: Refcount for this master object. + * @minor: Link back to minor char device we are master for. Immutable. + * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. + * @unique_len: Length of unique field. Protected by drm_global_mutex. + * @unique_size: Amount allocated. Protected by drm_global_mutex. + * @magiclist: Hash of used authentication tokens. Protected by struct_mutex. + * @magicfree: List of used authentication tokens. Protected by struct_mutex. + * @lock: DRI lock information. + * @driver_priv: Pointer to driver-private information. + */ struct drm_master { - - struct kref refcount; /* refcount for this master */ - - struct drm_minor *minor; /**< link back to minor we are a master for */ - - char *unique; /**< Unique identifier: e.g., busid */ - int unique_len; /**< Length of unique field */ - int unique_size; /**< amount allocated */ - - int blocked; /**< Blocked due to VC switch? */ - - /** \name Authentication */ - /*@{ */ + struct kref refcount; + struct drm_minor *minor; + char *unique; + int unique_len; + int unique_size; struct drm_open_hash magiclist; struct list_head magicfree; - /*@} */ - - struct drm_lock_data lock; /**< Information on hardware lock */ - - void *driver_priv; /**< Private structure for driver to use */ + struct drm_lock_data lock; + void *driver_priv; }; /* Size of ringbuffer for vblank timestamps. Just double-buffer @@ -1020,7 +1022,8 @@ struct drm_minor { struct list_head debugfs_list; struct mutex debugfs_lock; /* Protects debugfs_list. */ - struct drm_master *master; /* currently active master for this node */ + /* currently active master for this node. Protected by master_mutex */ + struct drm_master *master; struct drm_mode_group mode_group; }; @@ -1070,6 +1073,7 @@ struct drm_device { /*@{ */ spinlock_t count_lock; /**< For inuse, drm_device::open_count, drm_device::buf_use */ struct mutex struct_mutex; /**< For others */ + struct mutex master_mutex; /**< For drm_minor::master and drm_file::is_master */ /*@} */ /** \name Usage Counters */ From 4beb6d9fa6a74c189884be4913cfdd24fc8758ac Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Wed, 26 Feb 2014 15:51:57 +0100 Subject: [PATCH 07/16] drm: Add a function to get the ioctl flags Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/drm_drv.c | 18 ++++++++++++++++++ include/drm/drmP.h | 1 + 2 files changed, 19 insertions(+) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index cf2dfb790bf1e..03711d00aaaea 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -450,3 +450,21 @@ long drm_ioctl(struct file *filp, return retcode; } EXPORT_SYMBOL(drm_ioctl); + +/** + * drm_ioctl_flags - Check for core ioctl and return ioctl permission flags + * + * @nr: Ioctl number. + * @flags: Where to return the ioctl permission flags + */ +bool drm_ioctl_flags(unsigned int nr, unsigned int *flags) +{ + if ((nr >= DRM_COMMAND_END && nr < DRM_CORE_IOCTL_COUNT) || + (nr < DRM_COMMAND_BASE)) { + *flags = drm_ioctls[nr].flags; + return true; + } + + return false; +} +EXPORT_SYMBOL(drm_ioctl_flags); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 4e24a1a0daeb0..e97fc998374c7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1229,6 +1229,7 @@ extern long drm_ioctl(struct file *filp, extern long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); extern int drm_lastclose(struct drm_device *dev); +extern bool drm_ioctl_flags(unsigned int nr, unsigned int *flags); /* Device support (drm_fops.h) */ extern struct mutex drm_global_mutex; From 294adf7d86226c0e6abeb4475159b03aa315d56f Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 27 Feb 2014 12:34:51 +0100 Subject: [PATCH 08/16] drm/vmwgfx: Use a per-device semaphore for reservation protection Don't use a per-master semaphore (ttm lock) for reservation protection, but rather a per-device semaphore. This is needed since clients connecting using render nodes aren't master aware. The ttm lock used should probably be replaced with a reader-write semaphore once the function down_xx_interruptible() is available. Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c | 15 ++++++--------- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 5 +++++ drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 9 ++++----- drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 10 ++++------ drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++++++--------- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 15 ++++++--------- drivers/gpu/drm/vmwgfx/vmwgfx_shader.c | 5 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 14 ++++++-------- 11 files changed, 46 insertions(+), 58 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c index 1e80152674b50..701d5207def6f 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c @@ -462,7 +462,6 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data, struct vmw_resource *tmp; struct drm_vmw_context_arg *arg = (struct drm_vmw_context_arg *)data; struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; - struct vmw_master *vmaster = vmw_master(file_priv->master); int ret; @@ -474,7 +473,7 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data, if (unlikely(vmw_user_context_size == 0)) vmw_user_context_size = ttm_round_pot(sizeof(*ctx)) + 128; - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -521,7 +520,7 @@ int vmw_context_define_ioctl(struct drm_device *dev, void *data, out_err: vmw_resource_unreference(&res); out_unlock: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c index a75840211b3c9..70ddce8358b0b 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_dmabuf.c @@ -52,11 +52,10 @@ int vmw_dmabuf_to_placement(struct vmw_private *dev_priv, struct ttm_placement *placement, bool interruptible) { - struct vmw_master *vmaster = dev_priv->active_master; struct ttm_buffer_object *bo = &buf->base; int ret; - ret = ttm_write_lock(&vmaster->lock, interruptible); + ret = ttm_write_lock(&dev_priv->reservation_sem, interruptible); if (unlikely(ret != 0)) return ret; @@ -71,7 +70,7 @@ int vmw_dmabuf_to_placement(struct vmw_private *dev_priv, ttm_bo_unreserve(bo); err: - ttm_write_unlock(&vmaster->lock); + ttm_write_unlock(&dev_priv->reservation_sem); return ret; } @@ -95,12 +94,11 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv, struct vmw_dma_buffer *buf, bool pin, bool interruptible) { - struct vmw_master *vmaster = dev_priv->active_master; struct ttm_buffer_object *bo = &buf->base; struct ttm_placement *placement; int ret; - ret = ttm_write_lock(&vmaster->lock, interruptible); + ret = ttm_write_lock(&dev_priv->reservation_sem, interruptible); if (unlikely(ret != 0)) return ret; @@ -143,7 +141,7 @@ int vmw_dmabuf_to_vram_or_gmr(struct vmw_private *dev_priv, err_unreserve: ttm_bo_unreserve(bo); err: - ttm_write_unlock(&vmaster->lock); + ttm_write_unlock(&dev_priv->reservation_sem); return ret; } @@ -198,7 +196,6 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv, struct vmw_dma_buffer *buf, bool pin, bool interruptible) { - struct vmw_master *vmaster = dev_priv->active_master; struct ttm_buffer_object *bo = &buf->base; struct ttm_placement placement; int ret = 0; @@ -209,7 +206,7 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv, placement = vmw_vram_placement; placement.lpfn = bo->num_pages; - ret = ttm_write_lock(&vmaster->lock, interruptible); + ret = ttm_write_lock(&dev_priv->reservation_sem, interruptible); if (unlikely(ret != 0)) return ret; @@ -232,7 +229,7 @@ int vmw_dmabuf_to_start_of_vram(struct vmw_private *dev_priv, ttm_bo_unreserve(bo); err_unlock: - ttm_write_unlock(&vmaster->lock); + ttm_write_unlock(&dev_priv->reservation_sem); return ret; } diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index c35715f26f400..54cfeb6fc3b16 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -606,6 +606,7 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset) mutex_init(&dev_priv->release_mutex); mutex_init(&dev_priv->binding_mutex); rwlock_init(&dev_priv->resource_lock); + ttm_lock_init(&dev_priv->reservation_sem); for (i = vmw_res_context; i < vmw_res_max; ++i) { idr_init(&dev_priv->res_idr[i]); @@ -1175,12 +1176,11 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val, { struct vmw_private *dev_priv = container_of(nb, struct vmw_private, pm_nb); - struct vmw_master *vmaster = dev_priv->active_master; switch (val) { case PM_HIBERNATION_PREPARE: case PM_SUSPEND_PREPARE: - ttm_suspend_lock(&vmaster->lock); + ttm_suspend_lock(&dev_priv->reservation_sem); /** * This empties VRAM and unbinds all GMR bindings. @@ -1194,7 +1194,7 @@ static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val, case PM_POST_HIBERNATION: case PM_POST_SUSPEND: case PM_POST_RESTORE: - ttm_suspend_unlock(&vmaster->lock); + ttm_suspend_unlock(&dev_priv->reservation_sem); break; case PM_RESTORE_PREPARE: diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index 07831554dad7a..8c6b71f6cac97 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -486,6 +486,11 @@ struct vmw_private { struct mutex release_mutex; uint32_t num_3d_resources; + /* + * Replace this with an rwsem as soon as we have down_xx_interruptible() + */ + struct ttm_lock reservation_sem; + /* * Query processing. These members * are protected by the cmdbuf mutex. diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c index efb575a7996c2..931490b9cfed0 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c @@ -2712,7 +2712,6 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data, { struct vmw_private *dev_priv = vmw_priv(dev); struct drm_vmw_execbuf_arg *arg = (struct drm_vmw_execbuf_arg *)data; - struct vmw_master *vmaster = vmw_master(file_priv->master); int ret; /* @@ -2729,7 +2728,7 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data, return -EINVAL; } - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -2745,6 +2744,6 @@ int vmw_execbuf_ioctl(struct drm_device *dev, void *data, vmw_kms_cursor_post_execbuf(dev_priv); out_unlock: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c index ed5ce2a41bbf0..9699bd174ae4a 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c @@ -377,14 +377,13 @@ static int vmw_fb_create_bo(struct vmw_private *vmw_priv, ne_placement.lpfn = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; - /* interuptable? */ - ret = ttm_write_lock(&vmw_priv->fbdev_master.lock, false); - if (unlikely(ret != 0)) - return ret; + (void) ttm_write_lock(&vmw_priv->reservation_sem, false); vmw_bo = kmalloc(sizeof(*vmw_bo), GFP_KERNEL); - if (!vmw_bo) + if (!vmw_bo) { + ret = -ENOMEM; goto err_unlock; + } ret = vmw_dmabuf_init(vmw_priv, vmw_bo, size, &ne_placement, diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c index 47b70949bf3af..37881ecf5d7a9 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c @@ -226,7 +226,6 @@ int vmw_present_ioctl(struct drm_device *dev, void *data, struct drm_vmw_present_arg *arg = (struct drm_vmw_present_arg *)data; struct vmw_surface *surface; - struct vmw_master *vmaster = vmw_master(file_priv->master); struct drm_vmw_rect __user *clips_ptr; struct drm_vmw_rect *clips = NULL; struct drm_framebuffer *fb; @@ -271,7 +270,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data, } vfb = vmw_framebuffer_to_vfb(fb); - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) goto out_no_ttm_lock; @@ -291,7 +290,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data, vmw_surface_unreference(&surface); out_no_surface: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); out_no_ttm_lock: drm_framebuffer_unreference(fb); out_no_fb: @@ -311,7 +310,6 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data, struct drm_vmw_fence_rep __user *user_fence_rep = (struct drm_vmw_fence_rep __user *) (unsigned long)arg->fence_rep; - struct vmw_master *vmaster = vmw_master(file_priv->master); struct drm_vmw_rect __user *clips_ptr; struct drm_vmw_rect *clips = NULL; struct drm_framebuffer *fb; @@ -361,7 +359,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data, goto out_no_ttm_lock; } - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) goto out_no_ttm_lock; @@ -369,7 +367,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev, void *data, vfb, user_fence_rep, clips, num_clips); - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); out_no_ttm_lock: drm_framebuffer_unreference(fb); out_no_fb: diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 8a650413dea57..159af7ee111fa 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -596,7 +596,6 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer, unsigned num_clips) { struct vmw_private *dev_priv = vmw_priv(framebuffer->dev); - struct vmw_master *vmaster = vmw_master(file_priv->master); struct vmw_framebuffer_surface *vfbs = vmw_framebuffer_to_vfbs(framebuffer); struct drm_clip_rect norect; @@ -611,7 +610,7 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer, drm_modeset_lock_all(dev_priv->dev); - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) { drm_modeset_unlock_all(dev_priv->dev); return ret; @@ -632,7 +631,7 @@ static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer, flags, color, clips, num_clips, inc, NULL); - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); drm_modeset_unlock_all(dev_priv->dev); @@ -954,7 +953,6 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer, unsigned num_clips) { struct vmw_private *dev_priv = vmw_priv(framebuffer->dev); - struct vmw_master *vmaster = vmw_master(file_priv->master); struct vmw_framebuffer_dmabuf *vfbd = vmw_framebuffer_to_vfbd(framebuffer); struct drm_clip_rect norect; @@ -962,7 +960,7 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer, drm_modeset_lock_all(dev_priv->dev); - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) { drm_modeset_unlock_all(dev_priv->dev); return ret; @@ -989,7 +987,7 @@ static int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer, clips, num_clips, increment, NULL); } - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); drm_modeset_unlock_all(dev_priv->dev); @@ -2022,7 +2020,6 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data, struct vmw_private *dev_priv = vmw_priv(dev); struct drm_vmw_update_layout_arg *arg = (struct drm_vmw_update_layout_arg *)data; - struct vmw_master *vmaster = vmw_master(file_priv->master); void __user *user_rects; struct drm_vmw_rect *rects; unsigned rects_size; @@ -2030,7 +2027,7 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data, int i; struct drm_mode_config *mode_config = &dev->mode_config; - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -2072,6 +2069,6 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data, out_free: kfree(rects); out_unlock: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index 9757b57f8388d..30439cbeac2bd 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -676,10 +676,9 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data, struct drm_vmw_dmabuf_rep *rep = &arg->rep; struct vmw_dma_buffer *dma_buf; uint32_t handle; - struct vmw_master *vmaster = vmw_master(file_priv->master); int ret; - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -696,7 +695,7 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data, vmw_dmabuf_unreference(&dma_buf); out_no_dmabuf: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } @@ -873,7 +872,6 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data, struct vmw_resource *tmp; struct drm_vmw_stream_arg *arg = (struct drm_vmw_stream_arg *)data; struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; - struct vmw_master *vmaster = vmw_master(file_priv->master); int ret; /* @@ -884,7 +882,7 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data, if (unlikely(vmw_user_stream_size == 0)) vmw_user_stream_size = ttm_round_pot(sizeof(*stream)) + 128; - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -932,7 +930,7 @@ int vmw_stream_claim_ioctl(struct drm_device *dev, void *data, out_err: vmw_resource_unreference(&res); out_unlock: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } @@ -985,14 +983,13 @@ int vmw_dumb_create(struct drm_file *file_priv, struct drm_mode_create_dumb *args) { struct vmw_private *dev_priv = vmw_priv(dev); - struct vmw_master *vmaster = vmw_master(file_priv->master); struct vmw_dma_buffer *dma_buf; int ret; args->pitch = args->width * ((args->bpp + 7) / 8); args->size = args->pitch * args->height; - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -1004,7 +1001,7 @@ int vmw_dumb_create(struct drm_file *file_priv, vmw_dmabuf_unreference(&dma_buf); out_no_dmabuf: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c index ee3856578a125..c1559eeaffe9f 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c @@ -449,7 +449,6 @@ int vmw_shader_define_ioctl(struct drm_device *dev, void *data, struct drm_vmw_shader_create_arg *arg = (struct drm_vmw_shader_create_arg *)data; struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; - struct vmw_master *vmaster = vmw_master(file_priv->master); struct vmw_dma_buffer *buffer = NULL; SVGA3dShaderType shader_type; int ret; @@ -487,14 +486,14 @@ int vmw_shader_define_ioctl(struct drm_device *dev, void *data, goto out_bad_arg; } - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) goto out_bad_arg; ret = vmw_shader_alloc(dev_priv, buffer, arg->size, arg->offset, shader_type, tfile, &arg->shader_handle); - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); out_bad_arg: vmw_dmabuf_unreference(&buffer); return ret; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index e7af580ab977f..aac243b9ec30b 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -697,7 +697,6 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, struct vmw_surface_offset *cur_offset; uint32_t num_sizes; uint32_t size; - struct vmw_master *vmaster = vmw_master(file_priv->master); const struct svga3d_surface_desc *desc; if (unlikely(vmw_user_surface_size == 0)) @@ -723,7 +722,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, return -EINVAL; } - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -862,7 +861,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, rep->sid = user_srf->prime.base.hash.key; vmw_resource_unreference(&res); - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return 0; out_no_copy: kfree(srf->offsets); @@ -873,7 +872,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, out_no_user_srf: ttm_mem_global_free(vmw_mem_glob(dev_priv), size); out_unlock: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } @@ -1173,7 +1172,6 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; int ret; uint32_t size; - struct vmw_master *vmaster = vmw_master(file_priv->master); const struct svga3d_surface_desc *desc; uint32_t backup_handle; @@ -1189,7 +1187,7 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, return -EINVAL; } - ret = ttm_read_lock(&vmaster->lock, true); + ret = ttm_read_lock(&dev_priv->reservation_sem, true); if (unlikely(ret != 0)) return ret; @@ -1283,12 +1281,12 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, vmw_resource_unreference(&res); - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return 0; out_no_user_srf: ttm_mem_global_free(vmw_mem_glob(dev_priv), size); out_unlock: - ttm_read_unlock(&vmaster->lock); + ttm_read_unlock(&dev_priv->reservation_sem); return ret; } From 64190bded3c84ed988ff620ec2f4cd858b53426c Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 27 Feb 2014 12:56:08 +0100 Subject: [PATCH 09/16] drm/vmwgfx: Reinstate and tighten security around legacy master model The following restrictions affect clients connecting using legacy nodes: *) Masters that have dropped master privilieges are not considered authenticated until they regain master privileges. *) Clients whose master have dropped master privileges block interruptibly on ioctls requiring authentication until their master regains master privileges. If their master exits, they are killed. This is primarily designed to prevent clients authenticated with one master to access data from clients authenticated with another master. (Think fast user-switching or data sniffers enabled while X is vt-switched). Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 94 +++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 54cfeb6fc3b16..8fdbe26a98d2c 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -982,12 +982,70 @@ static int vmw_driver_open(struct drm_device *dev, struct drm_file *file_priv) return ret; } -static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd, - unsigned long arg) +static struct vmw_master *vmw_master_check(struct drm_device *dev, + struct drm_file *file_priv, + unsigned int flags) +{ + int ret; + struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv); + struct vmw_master *vmaster; + + if (file_priv->minor->type != DRM_MINOR_LEGACY || + !(flags & DRM_AUTH)) + return NULL; + + ret = mutex_lock_interruptible(&dev->master_mutex); + if (unlikely(ret != 0)) + return ERR_PTR(-ERESTARTSYS); + + if (file_priv->is_master) { + mutex_unlock(&dev->master_mutex); + return NULL; + } + + /* + * Check if we were previously master, but now dropped. + */ + if (vmw_fp->locked_master) { + mutex_unlock(&dev->master_mutex); + DRM_ERROR("Dropped master trying to access ioctl that " + "requires authentication.\n"); + return ERR_PTR(-EACCES); + } + mutex_unlock(&dev->master_mutex); + + /* + * Taking the drm_global_mutex after the TTM lock might deadlock + */ + if (!(flags & DRM_UNLOCKED)) { + DRM_ERROR("Refusing locked ioctl access.\n"); + return ERR_PTR(-EDEADLK); + } + + /* + * Take the TTM lock. Possibly sleep waiting for the authenticating + * master to become master again, or for a SIGTERM if the + * authenticating master exits. + */ + vmaster = vmw_master(file_priv->master); + ret = ttm_read_lock(&vmaster->lock, true); + if (unlikely(ret != 0)) + vmaster = ERR_PTR(ret); + + return vmaster; +} + +static long vmw_generic_ioctl(struct file *filp, unsigned int cmd, + unsigned long arg, + long (*ioctl_func)(struct file *, unsigned int, + unsigned long)) { struct drm_file *file_priv = filp->private_data; struct drm_device *dev = file_priv->minor->dev; unsigned int nr = DRM_IOCTL_NR(cmd); + struct vmw_master *vmaster; + unsigned int flags; + long ret; /* * Do extra checking on driver private ioctls. @@ -996,18 +1054,44 @@ static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd, if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END) && (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls)) { const struct drm_ioctl_desc *ioctl = - &vmw_ioctls[nr - DRM_COMMAND_BASE]; + &vmw_ioctls[nr - DRM_COMMAND_BASE]; if (unlikely(ioctl->cmd_drv != cmd)) { DRM_ERROR("Invalid command format, ioctl %d\n", nr - DRM_COMMAND_BASE); return -EINVAL; } + flags = ioctl->flags; + } else if (!drm_ioctl_flags(nr, &flags)) + return -EINVAL; + + vmaster = vmw_master_check(dev, file_priv, flags); + if (unlikely(IS_ERR(vmaster))) { + DRM_INFO("IOCTL ERROR %d\n", nr); + return PTR_ERR(vmaster); } - return drm_ioctl(filp, cmd, arg); + ret = ioctl_func(filp, cmd, arg); + if (vmaster) + ttm_read_unlock(&vmaster->lock); + + return ret; +} + +static long vmw_unlocked_ioctl(struct file *filp, unsigned int cmd, + unsigned long arg) +{ + return vmw_generic_ioctl(filp, cmd, arg, &drm_ioctl); } +#ifdef CONFIG_COMPAT +static long vmw_compat_ioctl(struct file *filp, unsigned int cmd, + unsigned long arg) +{ + return vmw_generic_ioctl(filp, cmd, arg, &drm_compat_ioctl); +} +#endif + static void vmw_lastclose(struct drm_device *dev) { struct drm_crtc *crtc; @@ -1315,7 +1399,7 @@ static const struct file_operations vmwgfx_driver_fops = { .poll = vmw_fops_poll, .read = vmw_fops_read, #if defined(CONFIG_COMPAT) - .compat_ioctl = drm_compat_ioctl, + .compat_ioctl = vmw_compat_ioctl, #endif .llseek = noop_llseek, }; From 4649926d043205d3693551b36e2ed88a264177a5 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 27 Feb 2014 13:24:17 +0100 Subject: [PATCH 10/16] drm/vmwgfx: Drop authentication requirement on UNREF ioctls These ioctls will anyway only succeed if the client previously opened referenced the object. Furthermore, closing the client would implicitly execute the same action. This prevents clients from blocking on UNREF if their master dropped, and will allow masters to UNREF after dropping master privileges. Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 8fdbe26a98d2c..de8a9dc25f074 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -146,7 +146,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_ALLOC_DMABUF, vmw_dmabuf_alloc_ioctl, DRM_AUTH | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_UNREF_DMABUF, vmw_dmabuf_unref_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_CURSOR_BYPASS, vmw_kms_cursor_bypass_ioctl, DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED), @@ -161,11 +161,11 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl, DRM_AUTH | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_UNREF_CONTEXT, vmw_context_destroy_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_CREATE_SURFACE, vmw_surface_define_ioctl, DRM_AUTH | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_UNREF_SURFACE, vmw_surface_destroy_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl, DRM_AUTH | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, @@ -176,7 +176,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { vmw_fence_obj_signaled_ioctl, DRM_AUTH | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_FENCE_EVENT, vmw_fence_event_ioctl, DRM_AUTH | DRM_UNLOCKED), @@ -197,7 +197,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_AUTH | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_UNREF_SHADER, vmw_shader_destroy_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE, vmw_gb_surface_define_ioctl, DRM_AUTH | DRM_UNLOCKED), From adebcb20e4ca9330a293c294e1f104edba21dbaf Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Tue, 18 Mar 2014 15:00:56 +0100 Subject: [PATCH 11/16] drm/vmwgfx: Allow prime fds in the surface reference ioctls Allow prime fds and at the same time block legacy handles for render-nodes in the surface reference ioctls. This means these ioctls can be used directly from prime-aware clients, and that they can be called from render-nodes. Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 105 ++++++++++++++++-------- include/uapi/drm/vmwgfx_drm.h | 12 ++- 2 files changed, 81 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index aac243b9ec30b..d50cd76378c47 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -876,6 +876,64 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, return ret; } + +static int +vmw_surface_handle_reference(struct vmw_private *dev_priv, + struct drm_file *file_priv, + uint32_t u_handle, + enum drm_vmw_handle_type handle_type, + struct ttm_base_object **base_p) +{ + struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; + uint32_t handle; + struct ttm_base_object *base; + int ret; + + if (handle_type == DRM_VMW_HANDLE_PRIME) { + ret = ttm_prime_fd_to_handle(tfile, u_handle, &handle); + if (unlikely(ret != 0)) + return ret; + } else { + if (unlikely(drm_is_render_client(file_priv))) { + DRM_ERROR("Render client refused legacy " + "surface reference.\n"); + return -EACCES; + } + handle = u_handle; + } + + ret = -EINVAL; + base = ttm_base_object_lookup_for_ref(dev_priv->tdev, handle); + if (unlikely(base == NULL)) { + DRM_ERROR("Could not find surface to reference.\n"); + goto out_no_lookup; + } + + if (unlikely(ttm_base_object_type(base) != VMW_RES_SURFACE)) { + DRM_ERROR("Referenced object is not a surface.\n"); + goto out_bad_resource; + } + + if (handle_type != DRM_VMW_HANDLE_PRIME) { + ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL); + if (unlikely(ret != 0)) { + DRM_ERROR("Could not add a reference to a surface.\n"); + goto out_bad_resource; + } + } + + *base_p = base; + return 0; + +out_bad_resource: + ttm_base_object_unref(&base); +out_no_lookup: + if (handle_type == DRM_VMW_HANDLE_PRIME) + (void) ttm_ref_object_base_unref(tfile, handle, TTM_REF_USAGE); + + return ret; +} + /** * vmw_user_surface_define_ioctl - Ioctl function implementing * the user surface reference functionality. @@ -897,27 +955,16 @@ int vmw_surface_reference_ioctl(struct drm_device *dev, void *data, struct vmw_user_surface *user_srf; struct drm_vmw_size __user *user_sizes; struct ttm_base_object *base; - int ret = -EINVAL; - - base = ttm_base_object_lookup_for_ref(dev_priv->tdev, req->sid); - if (unlikely(base == NULL)) { - DRM_ERROR("Could not find surface to reference.\n"); - return -EINVAL; - } + int ret; - if (unlikely(ttm_base_object_type(base) != VMW_RES_SURFACE)) - goto out_bad_resource; + ret = vmw_surface_handle_reference(dev_priv, file_priv, req->sid, + req->handle_type, &base); + if (unlikely(ret != 0)) + return ret; user_srf = container_of(base, struct vmw_user_surface, prime.base); srf = &user_srf->srf; - ret = ttm_ref_object_add(tfile, &user_srf->prime.base, - TTM_REF_USAGE, NULL); - if (unlikely(ret != 0)) { - DRM_ERROR("Could not add a reference to a surface.\n"); - goto out_no_reference; - } - rep->flags = srf->flags; rep->format = srf->format; memcpy(rep->mip_levels, srf->mip_levels, sizeof(srf->mip_levels)); @@ -930,10 +977,10 @@ int vmw_surface_reference_ioctl(struct drm_device *dev, void *data, if (unlikely(ret != 0)) { DRM_ERROR("copy_to_user failed %p %u\n", user_sizes, srf->num_sizes); + ttm_ref_object_base_unref(tfile, base->hash.key, TTM_REF_USAGE); ret = -EFAULT; } -out_bad_resource: -out_no_reference: + ttm_base_object_unref(&base); return ret; @@ -1313,14 +1360,10 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data, uint32_t backup_handle; int ret = -EINVAL; - base = ttm_base_object_lookup_for_ref(dev_priv->tdev, req->sid); - if (unlikely(base == NULL)) { - DRM_ERROR("Could not find surface to reference.\n"); - return -EINVAL; - } - - if (unlikely(ttm_base_object_type(base) != VMW_RES_SURFACE)) - goto out_bad_resource; + ret = vmw_surface_handle_reference(dev_priv, file_priv, req->sid, + req->handle_type, &base); + if (unlikely(ret != 0)) + return ret; user_srf = container_of(base, struct vmw_user_surface, prime.base); srf = &user_srf->srf; @@ -1329,13 +1372,6 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data, goto out_bad_resource; } - ret = ttm_ref_object_add(tfile, &user_srf->prime.base, - TTM_REF_USAGE, NULL); - if (unlikely(ret != 0)) { - DRM_ERROR("Could not add a reference to a GB surface.\n"); - goto out_bad_resource; - } - mutex_lock(&dev_priv->cmdbuf_mutex); /* Protect res->backup */ ret = vmw_user_dmabuf_reference(tfile, srf->res.backup, &backup_handle); @@ -1344,8 +1380,7 @@ int vmw_gb_surface_reference_ioctl(struct drm_device *dev, void *data, if (unlikely(ret != 0)) { DRM_ERROR("Could not add a reference to a GB surface " "backup buffer.\n"); - (void) ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile, - req->sid, + (void) ttm_ref_object_base_unref(tfile, base->hash.key, TTM_REF_USAGE); goto out_bad_resource; } diff --git a/include/uapi/drm/vmwgfx_drm.h b/include/uapi/drm/vmwgfx_drm.h index 87792a5fee3ba..4fc66f6b12ced 100644 --- a/include/uapi/drm/vmwgfx_drm.h +++ b/include/uapi/drm/vmwgfx_drm.h @@ -89,6 +89,15 @@ #define DRM_VMW_PARAM_MAX_MOB_MEMORY 9 #define DRM_VMW_PARAM_MAX_MOB_SIZE 10 +/** + * enum drm_vmw_handle_type - handle type for ref ioctls + * + */ +enum drm_vmw_handle_type { + DRM_VMW_HANDLE_LEGACY = 0, + DRM_VMW_HANDLE_PRIME = 1 +}; + /** * struct drm_vmw_getparam_arg * @@ -177,6 +186,7 @@ struct drm_vmw_surface_create_req { * struct drm_wmv_surface_arg * * @sid: Surface id of created surface or surface to destroy or reference. + * @handle_type: Handle type for DRM_VMW_REF_SURFACE Ioctl. * * Output data from the DRM_VMW_CREATE_SURFACE Ioctl. * Input argument to the DRM_VMW_UNREF_SURFACE Ioctl. @@ -185,7 +195,7 @@ struct drm_vmw_surface_create_req { struct drm_vmw_surface_arg { int32_t sid; - uint32_t pad64; + enum drm_vmw_handle_type handle_type; }; /** From 6d10aab8f0658d624e5aafcd52b70afa23a75e5e Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Wed, 19 Mar 2014 10:45:11 +0100 Subject: [PATCH 12/16] drm/vmwgfx: Tighten security around surface sharing v2 If using legacy (non-prime) surface sharing, only allow surfaces to be shared between clients with the same master. This will block malicious clients from peeking at contents at surfaces from other (possibly vt-switched) masters. v2: s/legacy_client/primary_client/ Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index d50cd76378c47..4ecdbf3e59da2 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -36,11 +36,13 @@ * @base: The TTM base object handling user-space visibility. * @srf: The surface metadata. * @size: TTM accounting size for the surface. + * @master: master of the creating client. Used for security check. */ struct vmw_user_surface { struct ttm_prime_object prime; struct vmw_surface srf; uint32_t size; + struct drm_master *master; }; /** @@ -624,6 +626,8 @@ static void vmw_user_surface_free(struct vmw_resource *res) struct vmw_private *dev_priv = srf->res.dev_priv; uint32_t size = user_srf->size; + if (user_srf->master) + drm_master_put(&user_srf->master); kfree(srf->offsets); kfree(srf->sizes); kfree(srf->snooper.image); @@ -819,6 +823,8 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data, user_srf->prime.base.shareable = false; user_srf->prime.base.tfile = NULL; + if (drm_is_primary_client(file_priv)) + user_srf->master = drm_master_get(file_priv->master); /** * From this point, the generic resource management functions @@ -885,6 +891,7 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, struct ttm_base_object **base_p) { struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; + struct vmw_user_surface *user_srf; uint32_t handle; struct ttm_base_object *base; int ret; @@ -915,6 +922,21 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, } if (handle_type != DRM_VMW_HANDLE_PRIME) { + user_srf = container_of(base, struct vmw_user_surface, + prime.base); + + /* + * Make sure the surface creator has the same + * authenticating master. + */ + if (drm_is_primary_client(file_priv) && + user_srf->master != file_priv->master) { + DRM_ERROR("Trying to reference surface outside of" + " master domain.\n"); + ret = -EACCES; + goto out_bad_resource; + } + ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL); if (unlikely(ret != 0)) { DRM_ERROR("Could not add a reference to a surface.\n"); @@ -1273,6 +1295,8 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data, user_srf->prime.base.shareable = false; user_srf->prime.base.tfile = NULL; + if (drm_is_primary_client(file_priv)) + user_srf->master = drm_master_get(file_priv->master); /** * From this point, the generic resource management functions From 0d3215e3857ab679f74c9b26b7e711955c9d0438 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Wed, 19 Mar 2014 13:23:20 +0100 Subject: [PATCH 13/16] drm/ttm: Add a ttm_ref_object_exists function A function to be used to check whether a caller has put a ref object (opened) a struct ttm_base_object Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/ttm/ttm_object.c | 46 ++++++++++++++++++++++++++++++++ include/drm/ttm/ttm_object.h | 4 +++ 2 files changed, 50 insertions(+) diff --git a/drivers/gpu/drm/ttm/ttm_object.c b/drivers/gpu/drm/ttm/ttm_object.c index 53b51c4e671a7..d2a0533527896 100644 --- a/drivers/gpu/drm/ttm/ttm_object.c +++ b/drivers/gpu/drm/ttm/ttm_object.c @@ -270,6 +270,52 @@ ttm_base_object_lookup_for_ref(struct ttm_object_device *tdev, uint32_t key) } EXPORT_SYMBOL(ttm_base_object_lookup_for_ref); +/** + * ttm_ref_object_exists - Check whether a caller has a valid ref object + * (has opened) a base object. + * + * @tfile: Pointer to a struct ttm_object_file identifying the caller. + * @base: Pointer to a struct base object. + * + * Checks wether the caller identified by @tfile has put a valid USAGE + * reference object on the base object identified by @base. + */ +bool ttm_ref_object_exists(struct ttm_object_file *tfile, + struct ttm_base_object *base) +{ + struct drm_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE]; + struct drm_hash_item *hash; + struct ttm_ref_object *ref; + + rcu_read_lock(); + if (unlikely(drm_ht_find_item_rcu(ht, base->hash.key, &hash) != 0)) + goto out_false; + + /* + * Verify that the ref object is really pointing to our base object. + * Our base object could actually be dead, and the ref object pointing + * to another base object with the same handle. + */ + ref = drm_hash_entry(hash, struct ttm_ref_object, hash); + if (unlikely(base != ref->obj)) + goto out_false; + + /* + * Verify that the ref->obj pointer was actually valid! + */ + rmb(); + if (unlikely(atomic_read(&ref->kref.refcount) == 0)) + goto out_false; + + rcu_read_unlock(); + return true; + + out_false: + rcu_read_unlock(); + return false; +} +EXPORT_SYMBOL(ttm_ref_object_exists); + int ttm_ref_object_add(struct ttm_object_file *tfile, struct ttm_base_object *base, enum ttm_ref_type ref_type, bool *existed) diff --git a/include/drm/ttm/ttm_object.h b/include/drm/ttm/ttm_object.h index 0097cc03034e1..ed953f98f0e14 100644 --- a/include/drm/ttm/ttm_object.h +++ b/include/drm/ttm/ttm_object.h @@ -244,6 +244,10 @@ extern void ttm_base_object_unref(struct ttm_base_object **p_base); extern int ttm_ref_object_add(struct ttm_object_file *tfile, struct ttm_base_object *base, enum ttm_ref_type ref_type, bool *existed); + +extern bool ttm_ref_object_exists(struct ttm_object_file *tfile, + struct ttm_base_object *base); + /** * ttm_ref_object_base_unref * From f6dfe73abf3ae528b8c631d37739e6d61894c0b2 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Wed, 19 Mar 2014 15:06:21 +0100 Subject: [PATCH 14/16] drm/vmwgfx: Tighten the security around buffer maps Make sure only buffer objects that are referenced by the client can be mapped. Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index 30439cbeac2bd..01d68f0a69dca 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -538,8 +538,13 @@ int vmw_user_dmabuf_verify_access(struct ttm_buffer_object *bo, return -EPERM; vmw_user_bo = vmw_user_dma_buffer(bo); - return (vmw_user_bo->prime.base.tfile == tfile || - vmw_user_bo->prime.base.shareable) ? 0 : -EPERM; + + /* Check that the caller has opened the object. */ + if (likely(ttm_ref_object_exists(tfile, &vmw_user_bo->prime.base))) + return 0; + + DRM_ERROR("Could not grant buffer access.\n"); + return -EPERM; } /** From 03f802636bca817623d3c2e93f95a27385b62d68 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 20 Mar 2014 13:06:34 +0100 Subject: [PATCH 15/16] drm/vmwgfx: Enable render nodes Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 43 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index de8a9dc25f074..c7009581bb23a 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -142,11 +142,11 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_GET_PARAM, vmw_getparam_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_ALLOC_DMABUF, vmw_dmabuf_alloc_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_DMABUF, vmw_dmabuf_unref_ioctl, - DRM_UNLOCKED), + DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CURSOR_BYPASS, vmw_kms_cursor_bypass_ioctl, DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED), @@ -159,29 +159,28 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_MASTER | DRM_CONTROL_ALLOW | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_CONTEXT, vmw_context_destroy_ioctl, - DRM_UNLOCKED), + DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_SURFACE, vmw_surface_define_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_SURFACE, vmw_surface_destroy_ioctl, - DRM_UNLOCKED), + DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_SIGNALED, vmw_fence_obj_signaled_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl, - DRM_UNLOCKED), - VMW_IOCTL_DEF(VMW_FENCE_EVENT, - vmw_fence_event_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_UNLOCKED | DRM_RENDER_ALLOW), + VMW_IOCTL_DEF(VMW_FENCE_EVENT, vmw_fence_event_ioctl, + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GET_3D_CAP, vmw_get_cap_3d_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), /* these allow direct access to the framebuffers mark as master only */ VMW_IOCTL_DEF(VMW_PRESENT, vmw_present_ioctl, @@ -194,19 +193,19 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_MASTER | DRM_UNLOCKED), VMW_IOCTL_DEF(VMW_CREATE_SHADER, vmw_shader_define_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_SHADER, vmw_shader_destroy_ioctl, - DRM_UNLOCKED), + DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE, vmw_gb_surface_define_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_REF, vmw_gb_surface_reference_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_SYNCCPU, vmw_user_dmabuf_synccpu_ioctl, - DRM_AUTH | DRM_UNLOCKED), + DRM_AUTH | DRM_UNLOCKED | DRM_RENDER_ALLOW), }; static struct pci_device_id vmw_pci_id_list[] = { @@ -1406,7 +1405,7 @@ static const struct file_operations vmwgfx_driver_fops = { static struct drm_driver driver = { .driver_features = DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | - DRIVER_MODESET | DRIVER_PRIME, + DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER, .load = vmw_driver_load, .unload = vmw_driver_unload, .lastclose = vmw_lastclose, From 03c5b8f077218bec50f1355b76dea405a7112878 Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Thu, 20 Mar 2014 13:07:44 +0100 Subject: [PATCH 16/16] drm/vmwgfx: Bump driver minor and date Signal availability of prime fd reference ioctls and render nodes. Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index 8c6b71f6cac97..6b252a887ae2d 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -40,9 +40,9 @@ #include #include "vmwgfx_fence.h" -#define VMWGFX_DRIVER_DATE "20140228" +#define VMWGFX_DRIVER_DATE "20140325" #define VMWGFX_DRIVER_MAJOR 2 -#define VMWGFX_DRIVER_MINOR 5 +#define VMWGFX_DRIVER_MINOR 6 #define VMWGFX_DRIVER_PATCHLEVEL 0 #define VMWGFX_FILE_PAGE_OFFSET 0x00100000 #define VMWGFX_FIFO_STATIC_SIZE (1024*1024)