[PATCH] drm: Fix an unwanted master inheritance v2

[PATCH] drm: Fix an unwanted master inheritance v2

[Thomas Hellstrom]

A client calling drmSetMaster() using a file descriptor that was opened
when another client was master would inherit the latter client's master
object and all its authenticated clients.

This is unwanted behaviour, and when this happens, instead allocate a
brand new master object for the client calling drmSetMaster().

Fixes a BUG() throw in vmw_master_set().

Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/drm_drv.c  |  5 +++
 drivers/gpu/drm/drm_fops.c | 84 ++++++++++++++++++++++++++++++----------------
 include/drm/drmP.h         |  6 ++++
 3 files changed, 67 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 9362609..7dd6728 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -160,6 +160,11 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
+	if (!file_priv->allowed_master) {
+		ret = drm_new_set_master(dev, file_priv);
+		goto out_unlock;
+	}
+
 	file_priv->minor->master = drm_master_get(file_priv->master);
 	file_priv->is_master = 1;
 	if (dev->driver->master_set) {
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c59ce4d..6b5625e 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -126,6 +126,60 @@ static int drm_cpu_valid(void)
 }
 
 /**
+ * drm_new_set_master - Allocate a new master object and become master for the
+ * associated master realm.
+ *
+ * @dev: The associated device.
+ * @fpriv: File private identifying the client.
+ *
+ * This function must be called with dev::struct_mutex held.
+ * Returns negative error code on failure. Zero on success.
+ */
+int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
+{
+	struct drm_master *old_master;
+	int ret;
+
+	lockdep_assert_held_once(&dev->master_mutex);
+
+	/* create a new master */
+	fpriv->minor->master = drm_master_create(fpriv->minor);
+	if (!fpriv->minor->master)
+		return -ENOMEM;
+
+	/* take another reference for the copy in the local file priv */
+	old_master = fpriv->master;
+	fpriv->master = drm_master_get(fpriv->minor->master);
+
+	if (dev->driver->master_create) {
+		ret = dev->driver->master_create(dev, fpriv->master);
+		if (ret)
+			goto out_err;
+	}
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, fpriv, true);
+		if (ret)
+			goto out_err;
+	}
+
+	fpriv->is_master = 1;
+	fpriv->allowed_master = 1;
+	fpriv->authenticated = 1;
+	if (old_master)
+		drm_master_put(&old_master);
+
+	return 0;
+
+out_err:
+	/* drop both references and restore old master on failure */
+	drm_master_put(&fpriv->minor->master);
+	drm_master_put(&fpriv->master);
+	fpriv->master = old_master;
+
+	return ret;
+}
+
+/**
  * Called whenever a process opens /dev/drm.
  *
  * \param filp file pointer.
@@ -189,35 +243,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	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) {
-			ret = -ENOMEM;
+		ret = drm_new_set_master(dev, priv);
+		if (ret)
 			goto out_close;
-		}
-
-		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;
-
-		if (dev->driver->master_create) {
-			ret = dev->driver->master_create(dev, priv->master);
-			if (ret) {
-				/* drop both references if this fails */
-				drm_master_put(&priv->minor->master);
-				drm_master_put(&priv->master);
-				goto out_close;
-			}
-		}
-		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);
-				goto out_close;
-			}
-		}
 	} 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 0b921ae..441b26e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -309,6 +309,11 @@ struct drm_file {
 	unsigned universal_planes:1;
 	/* true if client understands atomic properties */
 	unsigned atomic:1;
+	/*
+	 * This client is allowed to gain master privileges for @master.
+	 * Protected by struct drm_device::master_mutex.
+	 */
+	unsigned allowed_master:1;
 
 	struct pid *pid;
 	kuid_t uid;
@@ -910,6 +915,7 @@ extern int drm_open(struct inode *inode, struct file *filp);
 extern ssize_t drm_read(struct file *filp, char __user *buffer,
 			size_t count, loff_t *offset);
 extern int drm_release(struct inode *inode, struct file *filp);
+extern int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
 
 				/* Mapping support (drm_vm.h) */
 extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);
-- 
2.5.0

[PATCH] drm: Fix an unwanted master inheritance v2

[Thomas Hellstrom]

A client calling drmSetMaster() using a file descriptor that was opened
when another client was master would inherit the latter client's master
object and all its authenticated clients.

This is unwanted behaviour, and when this happens, instead allocate a
brand new master object for the client calling drmSetMaster().

Fixes a BUG() throw in vmw_master_set().

Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/drm_drv.c  |  5 +++
 drivers/gpu/drm/drm_fops.c | 84 ++++++++++++++++++++++++++++++----------------
 include/drm/drmP.h         |  6 ++++
 3 files changed, 67 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 9362609..7dd6728 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -160,6 +160,11 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
+	if (!file_priv->allowed_master) {
+		ret = drm_new_set_master(dev, file_priv);
+		goto out_unlock;
+	}
+
 	file_priv->minor->master = drm_master_get(file_priv->master);
 	file_priv->is_master = 1;
 	if (dev->driver->master_set) {
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index c59ce4d..6b5625e 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -126,6 +126,60 @@ static int drm_cpu_valid(void)
 }
 
 /**
+ * drm_new_set_master - Allocate a new master object and become master for the
+ * associated master realm.
+ *
+ * @dev: The associated device.
+ * @fpriv: File private identifying the client.
+ *
+ * This function must be called with dev::struct_mutex held.
+ * Returns negative error code on failure. Zero on success.
+ */
+int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
+{
+	struct drm_master *old_master;
+	int ret;
+
+	lockdep_assert_held_once(&dev->master_mutex);
+
+	/* create a new master */
+	fpriv->minor->master = drm_master_create(fpriv->minor);
+	if (!fpriv->minor->master)
+		return -ENOMEM;
+
+	/* take another reference for the copy in the local file priv */
+	old_master = fpriv->master;
+	fpriv->master = drm_master_get(fpriv->minor->master);
+
+	if (dev->driver->master_create) {
+		ret = dev->driver->master_create(dev, fpriv->master);
+		if (ret)
+			goto out_err;
+	}
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, fpriv, true);
+		if (ret)
+			goto out_err;
+	}
+
+	fpriv->is_master = 1;
+	fpriv->allowed_master = 1;
+	fpriv->authenticated = 1;
+	if (old_master)
+		drm_master_put(&old_master);
+
+	return 0;
+
+out_err:
+	/* drop both references and restore old master on failure */
+	drm_master_put(&fpriv->minor->master);
+	drm_master_put(&fpriv->master);
+	fpriv->master = old_master;
+
+	return ret;
+}
+
+/**
  * Called whenever a process opens /dev/drm.
  *
  * \param filp file pointer.
@@ -189,35 +243,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	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) {
-			ret = -ENOMEM;
+		ret = drm_new_set_master(dev, priv);
+		if (ret)
 			goto out_close;
-		}
-
-		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;
-
-		if (dev->driver->master_create) {
-			ret = dev->driver->master_create(dev, priv->master);
-			if (ret) {
-				/* drop both references if this fails */
-				drm_master_put(&priv->minor->master);
-				drm_master_put(&priv->master);
-				goto out_close;
-			}
-		}
-		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);
-				goto out_close;
-			}
-		}
 	} 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 0b921ae..441b26e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -309,6 +309,11 @@ struct drm_file {
 	unsigned universal_planes:1;
 	/* true if client understands atomic properties */
 	unsigned atomic:1;
+	/*
+	 * This client is allowed to gain master privileges for @master.
+	 * Protected by struct drm_device::master_mutex.
+	 */
+	unsigned allowed_master:1;
 
 	struct pid *pid;
 	kuid_t uid;
@@ -910,6 +915,7 @@ extern int drm_open(struct inode *inode, struct file *filp);
 extern ssize_t drm_read(struct file *filp, char __user *buffer,
 			size_t count, loff_t *offset);
 extern int drm_release(struct inode *inode, struct file *filp);
+extern int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
 
 				/* Mapping support (drm_vm.h) */
 extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix an unwanted master inheritance v2

[Jani Nikula]

On Wed, 02 Dec 2015, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> A client calling drmSetMaster() using a file descriptor that was opened
> when another client was master would inherit the latter client's master
> object and all its authenticated clients.
>
> This is unwanted behaviour, and when this happens, instead allocate a
> brand new master object for the client calling drmSetMaster().
>
> Fixes a BUG() throw in vmw_master_set().
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>

Drive-by bikeshedding, the actual change might look neater (and easier
to revert if something falls apart) if you extracted the
drm_new_set_master() abstraction as a separate non-functional prep
patch. Not insisting, just a thought.

BR,
Jani.



> ---
>  drivers/gpu/drm/drm_drv.c  |  5 +++
>  drivers/gpu/drm/drm_fops.c | 84 ++++++++++++++++++++++++++++++----------------
>  include/drm/drmP.h         |  6 ++++
>  3 files changed, 67 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9362609..7dd6728 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -160,6 +160,11 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>  		goto out_unlock;
>  	}
>  
> +	if (!file_priv->allowed_master) {
> +		ret = drm_new_set_master(dev, file_priv);
> +		goto out_unlock;
> +	}
> +
>  	file_priv->minor->master = drm_master_get(file_priv->master);
>  	file_priv->is_master = 1;
>  	if (dev->driver->master_set) {
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index c59ce4d..6b5625e 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -126,6 +126,60 @@ static int drm_cpu_valid(void)
>  }
>  
>  /**
> + * drm_new_set_master - Allocate a new master object and become master for the
> + * associated master realm.
> + *
> + * @dev: The associated device.
> + * @fpriv: File private identifying the client.
> + *
> + * This function must be called with dev::struct_mutex held.
> + * Returns negative error code on failure. Zero on success.
> + */
> +int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> +{
> +	struct drm_master *old_master;
> +	int ret;
> +
> +	lockdep_assert_held_once(&dev->master_mutex);
> +
> +	/* create a new master */
> +	fpriv->minor->master = drm_master_create(fpriv->minor);
> +	if (!fpriv->minor->master)
> +		return -ENOMEM;
> +
> +	/* take another reference for the copy in the local file priv */
> +	old_master = fpriv->master;
> +	fpriv->master = drm_master_get(fpriv->minor->master);
> +
> +	if (dev->driver->master_create) {
> +		ret = dev->driver->master_create(dev, fpriv->master);
> +		if (ret)
> +			goto out_err;
> +	}
> +	if (dev->driver->master_set) {
> +		ret = dev->driver->master_set(dev, fpriv, true);
> +		if (ret)
> +			goto out_err;
> +	}
> +
> +	fpriv->is_master = 1;
> +	fpriv->allowed_master = 1;
> +	fpriv->authenticated = 1;
> +	if (old_master)
> +		drm_master_put(&old_master);
> +
> +	return 0;
> +
> +out_err:
> +	/* drop both references and restore old master on failure */
> +	drm_master_put(&fpriv->minor->master);
> +	drm_master_put(&fpriv->master);
> +	fpriv->master = old_master;
> +
> +	return ret;
> +}
> +
> +/**
>   * Called whenever a process opens /dev/drm.
>   *
>   * \param filp file pointer.
> @@ -189,35 +243,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
>  	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) {
> -			ret = -ENOMEM;
> +		ret = drm_new_set_master(dev, priv);
> +		if (ret)
>  			goto out_close;
> -		}
> -
> -		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;
> -
> -		if (dev->driver->master_create) {
> -			ret = dev->driver->master_create(dev, priv->master);
> -			if (ret) {
> -				/* drop both references if this fails */
> -				drm_master_put(&priv->minor->master);
> -				drm_master_put(&priv->master);
> -				goto out_close;
> -			}
> -		}
> -		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);
> -				goto out_close;
> -			}
> -		}
>  	} 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 0b921ae..441b26e 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -309,6 +309,11 @@ struct drm_file {
>  	unsigned universal_planes:1;
>  	/* true if client understands atomic properties */
>  	unsigned atomic:1;
> +	/*
> +	 * This client is allowed to gain master privileges for @master.
> +	 * Protected by struct drm_device::master_mutex.
> +	 */
> +	unsigned allowed_master:1;
>  
>  	struct pid *pid;
>  	kuid_t uid;
> @@ -910,6 +915,7 @@ extern int drm_open(struct inode *inode, struct file *filp);
>  extern ssize_t drm_read(struct file *filp, char __user *buffer,
>  			size_t count, loff_t *offset);
>  extern int drm_release(struct inode *inode, struct file *filp);
> +extern int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
>  
>  				/* Mapping support (drm_vm.h) */
>  extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm: Fix an unwanted master inheritance v2

[Jani Nikula]

On Wed, 02 Dec 2015, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> A client calling drmSetMaster() using a file descriptor that was opened
> when another client was master would inherit the latter client's master
> object and all its authenticated clients.
>
> This is unwanted behaviour, and when this happens, instead allocate a
> brand new master object for the client calling drmSetMaster().
>
> Fixes a BUG() throw in vmw_master_set().
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>

Drive-by bikeshedding, the actual change might look neater (and easier
to revert if something falls apart) if you extracted the
drm_new_set_master() abstraction as a separate non-functional prep
patch. Not insisting, just a thought.

BR,
Jani.



> ---
>  drivers/gpu/drm/drm_drv.c  |  5 +++
>  drivers/gpu/drm/drm_fops.c | 84 ++++++++++++++++++++++++++++++----------------
>  include/drm/drmP.h         |  6 ++++
>  3 files changed, 67 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9362609..7dd6728 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -160,6 +160,11 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>  		goto out_unlock;
>  	}
>  
> +	if (!file_priv->allowed_master) {
> +		ret = drm_new_set_master(dev, file_priv);
> +		goto out_unlock;
> +	}
> +
>  	file_priv->minor->master = drm_master_get(file_priv->master);
>  	file_priv->is_master = 1;
>  	if (dev->driver->master_set) {
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index c59ce4d..6b5625e 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -126,6 +126,60 @@ static int drm_cpu_valid(void)
>  }
>  
>  /**
> + * drm_new_set_master - Allocate a new master object and become master for the
> + * associated master realm.
> + *
> + * @dev: The associated device.
> + * @fpriv: File private identifying the client.
> + *
> + * This function must be called with dev::struct_mutex held.
> + * Returns negative error code on failure. Zero on success.
> + */
> +int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> +{
> +	struct drm_master *old_master;
> +	int ret;
> +
> +	lockdep_assert_held_once(&dev->master_mutex);
> +
> +	/* create a new master */
> +	fpriv->minor->master = drm_master_create(fpriv->minor);
> +	if (!fpriv->minor->master)
> +		return -ENOMEM;
> +
> +	/* take another reference for the copy in the local file priv */
> +	old_master = fpriv->master;
> +	fpriv->master = drm_master_get(fpriv->minor->master);
> +
> +	if (dev->driver->master_create) {
> +		ret = dev->driver->master_create(dev, fpriv->master);
> +		if (ret)
> +			goto out_err;
> +	}
> +	if (dev->driver->master_set) {
> +		ret = dev->driver->master_set(dev, fpriv, true);
> +		if (ret)
> +			goto out_err;
> +	}
> +
> +	fpriv->is_master = 1;
> +	fpriv->allowed_master = 1;
> +	fpriv->authenticated = 1;
> +	if (old_master)
> +		drm_master_put(&old_master);
> +
> +	return 0;
> +
> +out_err:
> +	/* drop both references and restore old master on failure */
> +	drm_master_put(&fpriv->minor->master);
> +	drm_master_put(&fpriv->master);
> +	fpriv->master = old_master;
> +
> +	return ret;
> +}
> +
> +/**
>   * Called whenever a process opens /dev/drm.
>   *
>   * \param filp file pointer.
> @@ -189,35 +243,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
>  	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) {
> -			ret = -ENOMEM;
> +		ret = drm_new_set_master(dev, priv);
> +		if (ret)
>  			goto out_close;
> -		}
> -
> -		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;
> -
> -		if (dev->driver->master_create) {
> -			ret = dev->driver->master_create(dev, priv->master);
> -			if (ret) {
> -				/* drop both references if this fails */
> -				drm_master_put(&priv->minor->master);
> -				drm_master_put(&priv->master);
> -				goto out_close;
> -			}
> -		}
> -		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);
> -				goto out_close;
> -			}
> -		}
>  	} 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 0b921ae..441b26e 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -309,6 +309,11 @@ struct drm_file {
>  	unsigned universal_planes:1;
>  	/* true if client understands atomic properties */
>  	unsigned atomic:1;
> +	/*
> +	 * This client is allowed to gain master privileges for @master.
> +	 * Protected by struct drm_device::master_mutex.
> +	 */
> +	unsigned allowed_master:1;
>  
>  	struct pid *pid;
>  	kuid_t uid;
> @@ -910,6 +915,7 @@ extern int drm_open(struct inode *inode, struct file *filp);
>  extern ssize_t drm_read(struct file *filp, char __user *buffer,
>  			size_t count, loff_t *offset);
>  extern int drm_release(struct inode *inode, struct file *filp);
> +extern int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
>  
>  				/* Mapping support (drm_vm.h) */
>  extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm: Fix an unwanted master inheritance v2

[Thomas Hellstrom]

Hi,

On 12/03/2015 12:12 PM, Jani Nikula wrote:
> On Wed, 02 Dec 2015, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> A client calling drmSetMaster() using a file descriptor that was opened
>> when another client was master would inherit the latter client's master
>> object and all its authenticated clients.
>>
>> This is unwanted behaviour, and when this happens, instead allocate a
>> brand new master object for the client calling drmSetMaster().
>>
>> Fixes a BUG() throw in vmw_master_set().
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Drive-by bikeshedding, the actual change might look neater (and easier
> to revert if something falls apart) if you extracted the
> drm_new_set_master() abstraction as a separate non-functional prep
> patch. Not insisting, just a thought.
>
> BR,
> Jani.
While you're probably right, I prefer to have this as a single patch to
avoid ending up in a backporting nightmare for stable.

Thanks,
Thomas





>
>
>
>> ---
>>  drivers/gpu/drm/drm_drv.c  |  5 +++
>>  drivers/gpu/drm/drm_fops.c | 84 ++++++++++++++++++++++++++++++----------------
>>  include/drm/drmP.h         |  6 ++++
>>  3 files changed, 67 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 9362609..7dd6728 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -160,6 +160,11 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>>  		goto out_unlock;
>>  	}
>>  
>> +	if (!file_priv->allowed_master) {
>> +		ret = drm_new_set_master(dev, file_priv);
>> +		goto out_unlock;
>> +	}
>> +
>>  	file_priv->minor->master = drm_master_get(file_priv->master);
>>  	file_priv->is_master = 1;
>>  	if (dev->driver->master_set) {
>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> index c59ce4d..6b5625e 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -126,6 +126,60 @@ static int drm_cpu_valid(void)
>>  }
>>  
>>  /**
>> + * drm_new_set_master - Allocate a new master object and become master for the
>> + * associated master realm.
>> + *
>> + * @dev: The associated device.
>> + * @fpriv: File private identifying the client.
>> + *
>> + * This function must be called with dev::struct_mutex held.
>> + * Returns negative error code on failure. Zero on success.
>> + */
>> +int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>> +{
>> +	struct drm_master *old_master;
>> +	int ret;
>> +
>> +	lockdep_assert_held_once(&dev->master_mutex);
>> +
>> +	/* create a new master */
>> +	fpriv->minor->master = drm_master_create(fpriv->minor);
>> +	if (!fpriv->minor->master)
>> +		return -ENOMEM;
>> +
>> +	/* take another reference for the copy in the local file priv */
>> +	old_master = fpriv->master;
>> +	fpriv->master = drm_master_get(fpriv->minor->master);
>> +
>> +	if (dev->driver->master_create) {
>> +		ret = dev->driver->master_create(dev, fpriv->master);
>> +		if (ret)
>> +			goto out_err;
>> +	}
>> +	if (dev->driver->master_set) {
>> +		ret = dev->driver->master_set(dev, fpriv, true);
>> +		if (ret)
>> +			goto out_err;
>> +	}
>> +
>> +	fpriv->is_master = 1;
>> +	fpriv->allowed_master = 1;
>> +	fpriv->authenticated = 1;
>> +	if (old_master)
>> +		drm_master_put(&old_master);
>> +
>> +	return 0;
>> +
>> +out_err:
>> +	/* drop both references and restore old master on failure */
>> +	drm_master_put(&fpriv->minor->master);
>> +	drm_master_put(&fpriv->master);
>> +	fpriv->master = old_master;
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>>   * Called whenever a process opens /dev/drm.
>>   *
>>   * \param filp file pointer.
>> @@ -189,35 +243,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
>>  	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) {
>> -			ret = -ENOMEM;
>> +		ret = drm_new_set_master(dev, priv);
>> +		if (ret)
>>  			goto out_close;
>> -		}
>> -
>> -		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;
>> -
>> -		if (dev->driver->master_create) {
>> -			ret = dev->driver->master_create(dev, priv->master);
>> -			if (ret) {
>> -				/* drop both references if this fails */
>> -				drm_master_put(&priv->minor->master);
>> -				drm_master_put(&priv->master);
>> -				goto out_close;
>> -			}
>> -		}
>> -		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);
>> -				goto out_close;
>> -			}
>> -		}
>>  	} 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 0b921ae..441b26e 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -309,6 +309,11 @@ struct drm_file {
>>  	unsigned universal_planes:1;
>>  	/* true if client understands atomic properties */
>>  	unsigned atomic:1;
>> +	/*
>> +	 * This client is allowed to gain master privileges for @master.
>> +	 * Protected by struct drm_device::master_mutex.
>> +	 */
>> +	unsigned allowed_master:1;
>>  
>>  	struct pid *pid;
>>  	kuid_t uid;
>> @@ -910,6 +915,7 @@ extern int drm_open(struct inode *inode, struct file *filp);
>>  extern ssize_t drm_read(struct file *filp, char __user *buffer,
>>  			size_t count, loff_t *offset);
>>  extern int drm_release(struct inode *inode, struct file *filp);
>> +extern int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
>>  
>>  				/* Mapping support (drm_vm.h) */
>>  extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);

Re: [PATCH] drm: Fix an unwanted master inheritance v2

[Thomas Hellstrom]

Hi,

On 12/03/2015 12:12 PM, Jani Nikula wrote:
> On Wed, 02 Dec 2015, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> A client calling drmSetMaster() using a file descriptor that was opened
>> when another client was master would inherit the latter client's master
>> object and all its authenticated clients.
>>
>> This is unwanted behaviour, and when this happens, instead allocate a
>> brand new master object for the client calling drmSetMaster().
>>
>> Fixes a BUG() throw in vmw_master_set().
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Drive-by bikeshedding, the actual change might look neater (and easier
> to revert if something falls apart) if you extracted the
> drm_new_set_master() abstraction as a separate non-functional prep
> patch. Not insisting, just a thought.
>
> BR,
> Jani.
While you're probably right, I prefer to have this as a single patch to
avoid ending up in a backporting nightmare for stable.

Thanks,
Thomas





>
>
>
>> ---
>>  drivers/gpu/drm/drm_drv.c  |  5 +++
>>  drivers/gpu/drm/drm_fops.c | 84 ++++++++++++++++++++++++++++++----------------
>>  include/drm/drmP.h         |  6 ++++
>>  3 files changed, 67 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 9362609..7dd6728 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -160,6 +160,11 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>>  		goto out_unlock;
>>  	}
>>  
>> +	if (!file_priv->allowed_master) {
>> +		ret = drm_new_set_master(dev, file_priv);
>> +		goto out_unlock;
>> +	}
>> +
>>  	file_priv->minor->master = drm_master_get(file_priv->master);
>>  	file_priv->is_master = 1;
>>  	if (dev->driver->master_set) {
>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> index c59ce4d..6b5625e 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -126,6 +126,60 @@ static int drm_cpu_valid(void)
>>  }
>>  
>>  /**
>> + * drm_new_set_master - Allocate a new master object and become master for the
>> + * associated master realm.
>> + *
>> + * @dev: The associated device.
>> + * @fpriv: File private identifying the client.
>> + *
>> + * This function must be called with dev::struct_mutex held.
>> + * Returns negative error code on failure. Zero on success.
>> + */
>> +int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>> +{
>> +	struct drm_master *old_master;
>> +	int ret;
>> +
>> +	lockdep_assert_held_once(&dev->master_mutex);
>> +
>> +	/* create a new master */
>> +	fpriv->minor->master = drm_master_create(fpriv->minor);
>> +	if (!fpriv->minor->master)
>> +		return -ENOMEM;
>> +
>> +	/* take another reference for the copy in the local file priv */
>> +	old_master = fpriv->master;
>> +	fpriv->master = drm_master_get(fpriv->minor->master);
>> +
>> +	if (dev->driver->master_create) {
>> +		ret = dev->driver->master_create(dev, fpriv->master);
>> +		if (ret)
>> +			goto out_err;
>> +	}
>> +	if (dev->driver->master_set) {
>> +		ret = dev->driver->master_set(dev, fpriv, true);
>> +		if (ret)
>> +			goto out_err;
>> +	}
>> +
>> +	fpriv->is_master = 1;
>> +	fpriv->allowed_master = 1;
>> +	fpriv->authenticated = 1;
>> +	if (old_master)
>> +		drm_master_put(&old_master);
>> +
>> +	return 0;
>> +
>> +out_err:
>> +	/* drop both references and restore old master on failure */
>> +	drm_master_put(&fpriv->minor->master);
>> +	drm_master_put(&fpriv->master);
>> +	fpriv->master = old_master;
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>>   * Called whenever a process opens /dev/drm.
>>   *
>>   * \param filp file pointer.
>> @@ -189,35 +243,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
>>  	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) {
>> -			ret = -ENOMEM;
>> +		ret = drm_new_set_master(dev, priv);
>> +		if (ret)
>>  			goto out_close;
>> -		}
>> -
>> -		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;
>> -
>> -		if (dev->driver->master_create) {
>> -			ret = dev->driver->master_create(dev, priv->master);
>> -			if (ret) {
>> -				/* drop both references if this fails */
>> -				drm_master_put(&priv->minor->master);
>> -				drm_master_put(&priv->master);
>> -				goto out_close;
>> -			}
>> -		}
>> -		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);
>> -				goto out_close;
>> -			}
>> -		}
>>  	} 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 0b921ae..441b26e 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -309,6 +309,11 @@ struct drm_file {
>>  	unsigned universal_planes:1;
>>  	/* true if client understands atomic properties */
>>  	unsigned atomic:1;
>> +	/*
>> +	 * This client is allowed to gain master privileges for @master.
>> +	 * Protected by struct drm_device::master_mutex.
>> +	 */
>> +	unsigned allowed_master:1;
>>  
>>  	struct pid *pid;
>>  	kuid_t uid;
>> @@ -910,6 +915,7 @@ extern int drm_open(struct inode *inode, struct file *filp);
>>  extern ssize_t drm_read(struct file *filp, char __user *buffer,
>>  			size_t count, loff_t *offset);
>>  extern int drm_release(struct inode *inode, struct file *filp);
>> +extern int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
>>  
>>  				/* Mapping support (drm_vm.h) */
>>  extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix an unwanted master inheritance v2

[Jani Nikula]

On Thu, 03 Dec 2015, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> Hi,
>
> On 12/03/2015 12:12 PM, Jani Nikula wrote:
>> On Wed, 02 Dec 2015, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>>> A client calling drmSetMaster() using a file descriptor that was opened
>>> when another client was master would inherit the latter client's master
>>> object and all its authenticated clients.
>>>
>>> This is unwanted behaviour, and when this happens, instead allocate a
>>> brand new master object for the client calling drmSetMaster().
>>>
>>> Fixes a BUG() throw in vmw_master_set().
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> Drive-by bikeshedding, the actual change might look neater (and easier
>> to revert if something falls apart) if you extracted the
>> drm_new_set_master() abstraction as a separate non-functional prep
>> patch. Not insisting, just a thought.
>>
>> BR,
>> Jani.
> While you're probably right, I prefer to have this as a single patch to
> avoid ending up in a backporting nightmare for stable.

That's reasonable.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm: Fix an unwanted master inheritance v2

[Emil Velikov]

On 2 December 2015 at 17:24, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> A client calling drmSetMaster() using a file descriptor that was opened
> when another client was master would inherit the latter client's master
> object and all its authenticated clients.
>
> This is unwanted behaviour, and when this happens, instead allocate a
> brand new master object for the client calling drmSetMaster().
>
> Fixes a BUG() throw in vmw_master_set().
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Looks a lot better thanks!

Fwiw
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

-Emil

Re: [PATCH] drm: Fix an unwanted master inheritance v2

[Emil Velikov]

On 2 December 2015 at 17:24, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> A client calling drmSetMaster() using a file descriptor that was opened
> when another client was master would inherit the latter client's master
> object and all its authenticated clients.
>
> This is unwanted behaviour, and when this happens, instead allocate a
> brand new master object for the client calling drmSetMaster().
>
> Fixes a BUG() throw in vmw_master_set().
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Looks a lot better thanks!

Fwiw
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel