Commit c996fd0b authored by Thomas Hellstrom's avatar Thomas Hellstrom
Browse files

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: default avatarThomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: default avatarBrian Paul <brianp@vmware.com>
Reviewed-by: default avatarDavid Herrmann <dh.herrmann@gmail.com>
Acked-by: default avatarDaniel Vetter <daniel@ffwll.ch>
parent a12cd002

drivers/gpu/drm/drm_fops.c

+11 −11
Original line number Diff line number Diff line
@@ -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);

drivers/gpu/drm/drm_stub.c

+28 −15
Original line number Diff line number Diff line
@@ -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;



	if (file_priv->minor->master && file_priv->minor->master != file_priv->master)

		return -EINVAL;

		goto out_unlock;



	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 @@ err_minors: 
	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);

}

include/drm/drmP.h

+25 −21
Original line number Diff line number Diff line
@@ -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 <drm/drm_crtc.h>



/* 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 */

CRA Git | Maintained and supported by SUSTech CRA and CCSE