[PATCH] drm: Fix an unwanted master inheritance

[PATCH] drm: Fix an unwanted master inheritance

[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 it's authenticated clients.

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

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/drm_drv.c  | 12 +++++++
 drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
 include/drm/drmP.h         |  6 ++++
 3 files changed, 70 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 9362609..1f072ba 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
+	if (!file_priv->allowed_master) {
+		struct drm_master *saved_master = file_priv->master;
+
+		ret = drm_new_set_master(dev, file_priv);
+		if (ret)
+			file_priv->master = saved_master;
+		else
+			drm_master_put(&saved_master);
+
+		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..4b5c11c 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -126,6 +126,56 @@ 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)
+{
+	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;
+
+	fpriv->is_master = 1;
+	fpriv->allowed_master = 1;
+
+	/* take another reference for the copy in the local file priv */
+	fpriv->master = drm_master_get(fpriv->minor->master);
+
+	fpriv->authenticated = 1;
+
+	if (dev->driver->master_create) {
+		ret = dev->driver->master_create(dev, fpriv->master);
+		if (ret) {
+			/* drop both references if this fails */
+			drm_master_put(&fpriv->minor->master);
+			drm_master_put(&fpriv->master);
+			return ret;
+		}
+	}
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, fpriv, true);
+		if (ret) {
+			/* drop both references if this fails */
+			drm_master_put(&fpriv->minor->master);
+			drm_master_put(&fpriv->master);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/**
  * Called whenever a process opens /dev/drm.
  *
  * \param filp file pointer.
@@ -189,35 +239,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..566b59e 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

[Daniel Vetter]

On Mon, Nov 30, 2015 at 04:44:21AM -0800, Thomas Hellstrom 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 it's authenticated clients.
> 
> This is unwanted behaviour, and when this happens, instead allocate a
> brand new master object for the client calling drmSetMaster().
> 
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>

Imo makes sense. It would be great to have a testcase for this, and for
non-kms stuff igt now has support for generic testcases that can be run on
any driver. See for example intel-gpu-tools/tests/core_get_auth_client.c.

I or Daniel Stone can help out (on irc or mail) with that.
-Daniel

> ---
>  drivers/gpu/drm/drm_drv.c  | 12 +++++++
>  drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
>  include/drm/drmP.h         |  6 ++++
>  3 files changed, 70 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9362609..1f072ba 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>  		goto out_unlock;
>  	}
>  
> +	if (!file_priv->allowed_master) {
> +		struct drm_master *saved_master = file_priv->master;
> +
> +		ret = drm_new_set_master(dev, file_priv);
> +		if (ret)
> +			file_priv->master = saved_master;
> +		else
> +			drm_master_put(&saved_master);
> +
> +		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..4b5c11c 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -126,6 +126,56 @@ 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)
> +{
> +	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;
> +
> +	fpriv->is_master = 1;
> +	fpriv->allowed_master = 1;
> +
> +	/* take another reference for the copy in the local file priv */
> +	fpriv->master = drm_master_get(fpriv->minor->master);
> +
> +	fpriv->authenticated = 1;
> +
> +	if (dev->driver->master_create) {
> +		ret = dev->driver->master_create(dev, fpriv->master);
> +		if (ret) {
> +			/* drop both references if this fails */
> +			drm_master_put(&fpriv->minor->master);
> +			drm_master_put(&fpriv->master);
> +			return ret;
> +		}
> +	}
> +	if (dev->driver->master_set) {
> +		ret = dev->driver->master_set(dev, fpriv, true);
> +		if (ret) {
> +			/* drop both references if this fails */
> +			drm_master_put(&fpriv->minor->master);
> +			drm_master_put(&fpriv->master);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * Called whenever a process opens /dev/drm.
>   *
>   * \param filp file pointer.
> @@ -189,35 +239,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..566b59e 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
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

[Thomas Hellstrom]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

Hi,

On 11/30/2015 04:00 PM, Daniel Vetter wrote:
> On Mon, Nov 30, 2015 at 04:44:21AM -0800, Thomas Hellstrom 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 it's authenticated clients.
>>
>> This is unwanted behaviour, and when this happens, instead allocate a
>> brand new master object for the client calling drmSetMaster().
>>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Imo makes sense. It would be great to have a testcase for this, and for
> non-kms stuff igt now has support for generic testcases that can be run on
> any driver. See for example intel-gpu-tools/tests/core_get_auth_client.c.
>
> I or Daniel Stone can help out (on irc or mail) with that.
> -Daniel

Given that this crashes the kernel by vmwgfx throwing a BUG on some
versions of SLE,
while probably all other drivers don't care, except that it's a security
issue, A generic test case involving DRM clients leaking information
between master realms would unfortunately be too resource consuming to
put together for our minimal driver team ;).

Although I used the attached C program run as root to trigger the
behavior and unconditional kernel crash on vmwgfx. On the affected SLE
versions, fd1 would represent Xorg, fd2 would represent plymouthd.

/Thomas


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: drm_master_bug.c --]
[-- Type: text/x-csrc; name="drm_master_bug.c", Size: 441 bytes --]

#include <xf86drm.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>

int main()
{
    int fd1, fd2;

    fd1 = open("/dev/dri/card0", O_RDWR);
    if (fd1 < 0)
	exit(-1);

    fd2 = open("/dev/dri/card0", O_RDWR);
    if (fd2 < 0)
	exit(-1);

    (void) drmDropMaster(fd1);
    (void) drmSetMaster(fd2);

    close(fd2);
    close(fd1);
}

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
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

[Daniel Vetter]

On Mon, Nov 30, 2015 at 04:27:50PM +0100, Thomas Hellstrom wrote:
> Hi,
> 
> On 11/30/2015 04:00 PM, Daniel Vetter wrote:
> > On Mon, Nov 30, 2015 at 04:44:21AM -0800, Thomas Hellstrom 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 it's authenticated clients.
> >>
> >> This is unwanted behaviour, and when this happens, instead allocate a
> >> brand new master object for the client calling drmSetMaster().
> >>
> >> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > Imo makes sense. It would be great to have a testcase for this, and for
> > non-kms stuff igt now has support for generic testcases that can be run on
> > any driver. See for example intel-gpu-tools/tests/core_get_auth_client.c.
> >
> > I or Daniel Stone can help out (on irc or mail) with that.
> > -Daniel
> 
> Given that this crashes the kernel by vmwgfx throwing a BUG on some
> versions of SLE,
> while probably all other drivers don't care, except that it's a security
> issue, A generic test case involving DRM clients leaking information
> between master realms would unfortunately be too resource consuming to
> put together for our minimal driver team ;).
> 
> Although I used the attached C program run as root to trigger the
> behavior and unconditional kernel crash on vmwgfx. On the affected SLE
> versions, fd1 would represent Xorg, fd2 would represent plymouthd.
> 
> /Thomas
> 

> #include <xf86drm.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <stdio.h>
> 
> int main()
> {
>     int fd1, fd2;
> 
>     fd1 = open("/dev/dri/card0", O_RDWR);
>     if (fd1 < 0)
> 	exit(-1);
> 
>     fd2 = open("/dev/dri/card0", O_RDWR);
>     if (fd2 < 0)
> 	exit(-1);

I think if you open fd3 here an auth it with fd1 ...

>     (void) drmDropMaster(fd1);
>     (void) drmSetMaster(fd2);

and then check whether fd1 is still authenticated (and fail if so) it
should work as a testcase. Converting it over to igt would be trivial, I
can do that if you want. We also already have auth testcases in igt, so
should be at most a bit of copypasting to get it together.

Or did I miss a needed detail in how to repro this?
-Daniel

> 
>     close(fd2);
>     close(fd1);
> }


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
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

[Thomas Hellstrom]

On 11/30/2015 05:09 PM, Daniel Vetter wrote:
> On Mon, Nov 30, 2015 at 04:27:50PM +0100, Thomas Hellstrom wrote:
>> Hi,
>>
>> On 11/30/2015 04:00 PM, Daniel Vetter wrote:
>>> On Mon, Nov 30, 2015 at 04:44:21AM -0800, Thomas Hellstrom 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 it's authenticated clients.
>>>>
>>>> This is unwanted behaviour, and when this happens, instead allocate a
>>>> brand new master object for the client calling drmSetMaster().
>>>>
>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>>> Imo makes sense. It would be great to have a testcase for this, and for
>>> non-kms stuff igt now has support for generic testcases that can be run on
>>> any driver. See for example intel-gpu-tools/tests/core_get_auth_client.c.
>>>
>>> I or Daniel Stone can help out (on irc or mail) with that.
>>> -Daniel
>> Given that this crashes the kernel by vmwgfx throwing a BUG on some
>> versions of SLE,
>> while probably all other drivers don't care, except that it's a security
>> issue, A generic test case involving DRM clients leaking information
>> between master realms would unfortunately be too resource consuming to
>> put together for our minimal driver team ;).
>>
>> Although I used the attached C program run as root to trigger the
>> behavior and unconditional kernel crash on vmwgfx. On the affected SLE
>> versions, fd1 would represent Xorg, fd2 would represent plymouthd.
>>
>> /Thomas
>>
>> #include <xf86drm.h>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <unistd.h>
>> #include <stdlib.h>
>> #include <stdio.h>
>>
>> int main()
>> {
>>     int fd1, fd2;
>>
>>     fd1 = open("/dev/dri/card0", O_RDWR);
>>     if (fd1 < 0)
>> 	exit(-1);
>>
>>     fd2 = open("/dev/dri/card0", O_RDWR);
>>     if (fd2 < 0)
>> 	exit(-1);
> I think if you open fd3 here an auth it with fd1 ...
>
>>     (void) drmDropMaster(fd1);
>>     (void) drmSetMaster(fd2);
> and then check whether fd1 is still authenticated (and fail if so) it
> should work as a testcase. Converting it over to igt would be trivial, I
> can do that if you want. We also already have auth testcases in igt, so
> should be at most a bit of copypasting to get it together.
>
> Or did I miss a needed detail in how to repro this?
> -Daniel

Yes, an authenticated fd is always authenticated, no matter what master
is current. And core DRM leaves data isolation between master realms to
subsystems or drivers.

What we could do is to obtain an auth magic for fd3 from fd1 and try to
use it with fd2 to authenticate after master switch. That should work
without this patch, but not with is.

/Thomas


>
>>     close(fd2);
>>     close(fd1);
>> }
>


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

Re: [Pv-drivers] [PATCH] drm: Fix an unwanted master inheritance

[Sinclair Yeh]

Reviewed-by: Sinclair Yeh <syeh@vmware.com>

On Mon, Nov 30, 2015 at 04:44:21AM -0800, Thomas Hellstrom 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 it's authenticated clients.
> 
> This is unwanted behaviour, and when this happens, instead allocate a
> brand new master object for the client calling drmSetMaster().
> 
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  drivers/gpu/drm/drm_drv.c  | 12 +++++++
>  drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
>  include/drm/drmP.h         |  6 ++++
>  3 files changed, 70 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9362609..1f072ba 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>  		goto out_unlock;
>  	}
>  
> +	if (!file_priv->allowed_master) {
> +		struct drm_master *saved_master = file_priv->master;
> +
> +		ret = drm_new_set_master(dev, file_priv);
> +		if (ret)
> +			file_priv->master = saved_master;
> +		else
> +			drm_master_put(&saved_master);
> +
> +		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..4b5c11c 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -126,6 +126,56 @@ 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)
> +{
> +	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;
> +
> +	fpriv->is_master = 1;
> +	fpriv->allowed_master = 1;
> +
> +	/* take another reference for the copy in the local file priv */
> +	fpriv->master = drm_master_get(fpriv->minor->master);
> +
> +	fpriv->authenticated = 1;
> +
> +	if (dev->driver->master_create) {
> +		ret = dev->driver->master_create(dev, fpriv->master);
> +		if (ret) {
> +			/* drop both references if this fails */
> +			drm_master_put(&fpriv->minor->master);
> +			drm_master_put(&fpriv->master);
> +			return ret;
> +		}
> +	}
> +	if (dev->driver->master_set) {
> +		ret = dev->driver->master_set(dev, fpriv, true);
> +		if (ret) {
> +			/* drop both references if this fails */
> +			drm_master_put(&fpriv->minor->master);
> +			drm_master_put(&fpriv->master);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * Called whenever a process opens /dev/drm.
>   *
>   * \param filp file pointer.
> @@ -189,35 +239,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..566b59e 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
> 
> _______________________________________________
> Pv-drivers mailing list
> Pv-drivers@vmware.com
> http://mailman2.vmware.com/mailman/listinfo/pv-drivers
_______________________________________________
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

[Lukas Wunner]

Hi,

On Mon, Nov 30, 2015 at 04:27:50PM +0100, Thomas Hellstrom wrote:
> while probably all other drivers don't care, except that it's a security
> issue

Hm, I don't know what the security policy is for DRM-related issues
but shouldn't this be cc'ed to security@kernel.org so that it gets the
attention of security folks at distro vendors and is assigned a CVE?

Best regards,

Lukas
_______________________________________________
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

[Thomas Hellstrom]

Hi,

I'm not completely sure that many drivers except vmwgfx care about
inter-master DRM
information leaks, of which this is one. (For example I think most
drivers allow a bo flinked by a driver in one master realm (one user) to
be opened by a client in another master realm (another user)).

I think the common opinion is to ignore this and push for general
render-node usage. Should that not be the case, we can always forward
this. Note, however, that the impact for this particular issue should be
quite low because it requires the cooperation of a user-space client
with root privileges that is sloppy with timing....

/Thomas

On 11/30/2015 08:53 PM, Lukas Wunner wrote:
> Hi,
>
> On Mon, Nov 30, 2015 at 04:27:50PM +0100, Thomas Hellstrom wrote:
>> while probably all other drivers don't care, except that it's a security
>> issue
> Hm, I don't know what the security policy is for DRM-related issues
> but shouldn't this be cc'ed to security@kernel.org so that it gets the
> attention of security folks at distro vendors and is assigned a CVE?
>
> Best regards,
>
> Lukas


_______________________________________________
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

[Emil Velikov]

Hi Thomas,

Something doesn't feel quite right, please read on.

On 30 November 2015 at 12:44, 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 it's authenticated clients.
>
> This is unwanted behaviour, and when this happens, instead allocate a
> brand new master object for the client calling drmSetMaster().
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  drivers/gpu/drm/drm_drv.c  | 12 +++++++
>  drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
>  include/drm/drmP.h         |  6 ++++
>  3 files changed, 70 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9362609..1f072ba 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>                 goto out_unlock;
>         }
>
> +       if (!file_priv->allowed_master) {
> +               struct drm_master *saved_master = file_priv->master;
> +
> +               ret = drm_new_set_master(dev, file_priv);
> +               if (ret)
> +                       file_priv->master = saved_master;
Imho this shouldn'e belong here but in drm_new_set_master() - i.e. it
should unwind things on error. Similarly, although we restore the old
drm_master (below), we still have is_master, allowed_master and
authenticated set. Thus one can reuse the elevated credentials (is
this the right terminology?) despite that the ioctl has failed.

> +               else
> +                       drm_master_put(&saved_master);
> +
> +               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..4b5c11c 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -126,6 +126,56 @@ 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)
> +{
> +       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;
> +
> +       fpriv->is_master = 1;
> +       fpriv->allowed_master = 1;
> +
> +       /* take another reference for the copy in the local file priv */
> +       fpriv->master = drm_master_get(fpriv->minor->master);
> +
> +       fpriv->authenticated = 1;
> +
> +       if (dev->driver->master_create) {
> +               ret = dev->driver->master_create(dev, fpriv->master);
> +               if (ret) {
> +                       /* drop both references if this fails */
> +                       drm_master_put(&fpriv->minor->master);
> +                       drm_master_put(&fpriv->master);
> +                       return ret;
> +               }
> +       }
> +       if (dev->driver->master_set) {
> +               ret = dev->driver->master_set(dev, fpriv, true);
> +               if (ret) {
Afaics both of these callbacks are available from legacy(UMS) drivers
aren't they ? With the radeon UMS removal patches in the works, this
leaves vmwgfx.

Either way, perhaps we should set is_master, allowed_master and
authenticated after these two ? Or alternatively restore the original
values on error.

Did I miss something or the above sounds about right ?

Regards,
Emil
_______________________________________________
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

[Thomas Hellstrom]

On 12/01/2015 11:57 AM, Emil Velikov wrote:
> Hi Thomas,
>
> Something doesn't feel quite right, please read on.
>
> On 30 November 2015 at 12:44, 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 it's authenticated clients.
>>
>> This is unwanted behaviour, and when this happens, instead allocate a
>> brand new master object for the client calling drmSetMaster().
>>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> ---
>>  drivers/gpu/drm/drm_drv.c  | 12 +++++++
>>  drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
>>  include/drm/drmP.h         |  6 ++++
>>  3 files changed, 70 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 9362609..1f072ba 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>>                 goto out_unlock;
>>         }
>>
>> +       if (!file_priv->allowed_master) {
>> +               struct drm_master *saved_master = file_priv->master;
>> +
>> +               ret = drm_new_set_master(dev, file_priv);
>> +               if (ret)
>> +                       file_priv->master = saved_master;
> Imho this shouldn'e belong here but in drm_new_set_master() - i.e. it
> should unwind things on error. Similarly, although we restore the old
> drm_master (below), we still have is_master, allowed_master and
> authenticated set. Thus one can reuse the elevated credentials (is
> this the right terminology?) despite that the ioctl has failed.
>
>> +               else
>> +                       drm_master_put(&saved_master);
>> +
>> +               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..4b5c11c 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -126,6 +126,56 @@ 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)
>> +{
>> +       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;
>> +
>> +       fpriv->is_master = 1;
>> +       fpriv->allowed_master = 1;
>> +
>> +       /* take another reference for the copy in the local file priv */
>> +       fpriv->master = drm_master_get(fpriv->minor->master);
>> +
>> +       fpriv->authenticated = 1;
>> +
>> +       if (dev->driver->master_create) {
>> +               ret = dev->driver->master_create(dev, fpriv->master);
>> +               if (ret) {
>> +                       /* drop both references if this fails */
>> +                       drm_master_put(&fpriv->minor->master);
>> +                       drm_master_put(&fpriv->master);
>> +                       return ret;
>> +               }
>> +       }
>> +       if (dev->driver->master_set) {
>> +               ret = dev->driver->master_set(dev, fpriv, true);
>> +               if (ret) {
> Afaics both of these callbacks are available from legacy(UMS) drivers
> aren't they ? With the radeon UMS removal patches in the works, this
> leaves vmwgfx.
>
> Either way, perhaps we should set is_master, allowed_master and
> authenticated after these two ? Or alternatively restore the original
> values on error.
>
> Did I miss something or the above sounds about right ?
It does. I'll address this and resend.

Thanks for reviewing!

/Thomas



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

_______________________________________________
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

[Daniel Vetter]

On Tue, Dec 01, 2015 at 12:58:13PM +0100, Thomas Hellstrom wrote:
> On 12/01/2015 11:57 AM, Emil Velikov wrote:
> > Hi Thomas,
> >
> > Something doesn't feel quite right, please read on.
> >
> > On 30 November 2015 at 12:44, 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 it's authenticated clients.
> >>
> >> This is unwanted behaviour, and when this happens, instead allocate a
> >> brand new master object for the client calling drmSetMaster().
> >>
> >> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> >> ---
> >>  drivers/gpu/drm/drm_drv.c  | 12 +++++++
> >>  drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
> >>  include/drm/drmP.h         |  6 ++++
> >>  3 files changed, 70 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 9362609..1f072ba 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
> >>                 goto out_unlock;
> >>         }
> >>
> >> +       if (!file_priv->allowed_master) {
> >> +               struct drm_master *saved_master = file_priv->master;
> >> +
> >> +               ret = drm_new_set_master(dev, file_priv);
> >> +               if (ret)
> >> +                       file_priv->master = saved_master;
> > Imho this shouldn'e belong here but in drm_new_set_master() - i.e. it
> > should unwind things on error. Similarly, although we restore the old
> > drm_master (below), we still have is_master, allowed_master and
> > authenticated set. Thus one can reuse the elevated credentials (is
> > this the right terminology?) despite that the ioctl has failed.
> >
> >> +               else
> >> +                       drm_master_put(&saved_master);
> >> +
> >> +               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..4b5c11c 100644
> >> --- a/drivers/gpu/drm/drm_fops.c
> >> +++ b/drivers/gpu/drm/drm_fops.c
> >> @@ -126,6 +126,56 @@ 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)
> >> +{
> >> +       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;
> >> +
> >> +       fpriv->is_master = 1;
> >> +       fpriv->allowed_master = 1;
> >> +
> >> +       /* take another reference for the copy in the local file priv */
> >> +       fpriv->master = drm_master_get(fpriv->minor->master);
> >> +
> >> +       fpriv->authenticated = 1;
> >> +
> >> +       if (dev->driver->master_create) {
> >> +               ret = dev->driver->master_create(dev, fpriv->master);
> >> +               if (ret) {
> >> +                       /* drop both references if this fails */
> >> +                       drm_master_put(&fpriv->minor->master);
> >> +                       drm_master_put(&fpriv->master);
> >> +                       return ret;
> >> +               }
> >> +       }
> >> +       if (dev->driver->master_set) {
> >> +               ret = dev->driver->master_set(dev, fpriv, true);
> >> +               if (ret) {
> > Afaics both of these callbacks are available from legacy(UMS) drivers
> > aren't they ? With the radeon UMS removal patches in the works, this
> > leaves vmwgfx.
> >
> > Either way, perhaps we should set is_master, allowed_master and
> > authenticated after these two ? Or alternatively restore the original
> > values on error.
> >
> > Did I miss something or the above sounds about right ?
> It does. I'll address this and resend.

Just wanted to pull this in and noticed there's still this open. New
version incoming soon?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
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

[Thomas Hellstrom]

On 12/02/2015 04:54 PM, Daniel Vetter wrote:
> On Tue, Dec 01, 2015 at 12:58:13PM +0100, Thomas Hellstrom wrote:
>> On 12/01/2015 11:57 AM, Emil Velikov wrote:
>>> Hi Thomas,
>>>
>>> Something doesn't feel quite right, please read on.
>>>
>>> On 30 November 2015 at 12:44, 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 it's authenticated clients.
>>>>
>>>> This is unwanted behaviour, and when this happens, instead allocate a
>>>> brand new master object for the client calling drmSetMaster().
>>>>
>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_drv.c  | 12 +++++++
>>>>  drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
>>>>  include/drm/drmP.h         |  6 ++++
>>>>  3 files changed, 70 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 9362609..1f072ba 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>>>>                 goto out_unlock;
>>>>         }
>>>>
>>>> +       if (!file_priv->allowed_master) {
>>>> +               struct drm_master *saved_master = file_priv->master;
>>>> +
>>>> +               ret = drm_new_set_master(dev, file_priv);
>>>> +               if (ret)
>>>> +                       file_priv->master = saved_master;
>>> Imho this shouldn'e belong here but in drm_new_set_master() - i.e. it
>>> should unwind things on error. Similarly, although we restore the old
>>> drm_master (below), we still have is_master, allowed_master and
>>> authenticated set. Thus one can reuse the elevated credentials (is
>>> this the right terminology?) despite that the ioctl has failed.
>>>
>>>> +               else
>>>> +                       drm_master_put(&saved_master);
>>>> +
>>>> +               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..4b5c11c 100644
>>>> --- a/drivers/gpu/drm/drm_fops.c
>>>> +++ b/drivers/gpu/drm/drm_fops.c
>>>> @@ -126,6 +126,56 @@ 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)
>>>> +{
>>>> +       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;
>>>> +
>>>> +       fpriv->is_master = 1;
>>>> +       fpriv->allowed_master = 1;
>>>> +
>>>> +       /* take another reference for the copy in the local file priv */
>>>> +       fpriv->master = drm_master_get(fpriv->minor->master);
>>>> +
>>>> +       fpriv->authenticated = 1;
>>>> +
>>>> +       if (dev->driver->master_create) {
>>>> +               ret = dev->driver->master_create(dev, fpriv->master);
>>>> +               if (ret) {
>>>> +                       /* drop both references if this fails */
>>>> +                       drm_master_put(&fpriv->minor->master);
>>>> +                       drm_master_put(&fpriv->master);
>>>> +                       return ret;
>>>> +               }
>>>> +       }
>>>> +       if (dev->driver->master_set) {
>>>> +               ret = dev->driver->master_set(dev, fpriv, true);
>>>> +               if (ret) {
>>> Afaics both of these callbacks are available from legacy(UMS) drivers
>>> aren't they ? With the radeon UMS removal patches in the works, this
>>> leaves vmwgfx.
>>>
>>> Either way, perhaps we should set is_master, allowed_master and
>>> authenticated after these two ? Or alternatively restore the original
>>> values on error.
>>>
>>> Did I miss something or the above sounds about right ?
>> It does. I'll address this and resend.
> Just wanted to pull this in and noticed there's still this open. New
> version incoming soon?
>
> Thanks, Daniel
Hopefully tonight.

Home with sick children...

/Thomas

_______________________________________________
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

[Thomas Hellstrom]

On 12/02/2015 04:54 PM, Daniel Vetter wrote:
> On Tue, Dec 01, 2015 at 12:58:13PM +0100, Thomas Hellstrom wrote:
>> On 12/01/2015 11:57 AM, Emil Velikov wrote:
>>> Hi Thomas,
>>>
>>> Something doesn't feel quite right, please read on.
>>>
>>> On 30 November 2015 at 12:44, 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 it's authenticated clients.
>>>>
>>>> This is unwanted behaviour, and when this happens, instead allocate a
>>>> brand new master object for the client calling drmSetMaster().
>>>>
>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_drv.c  | 12 +++++++
>>>>  drivers/gpu/drm/drm_fops.c | 80 ++++++++++++++++++++++++++++++----------------
>>>>  include/drm/drmP.h         |  6 ++++
>>>>  3 files changed, 70 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 9362609..1f072ba 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -160,6 +160,18 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>>>>                 goto out_unlock;
>>>>         }
>>>>
>>>> +       if (!file_priv->allowed_master) {
>>>> +               struct drm_master *saved_master = file_priv->master;
>>>> +
>>>> +               ret = drm_new_set_master(dev, file_priv);
>>>> +               if (ret)
>>>> +                       file_priv->master = saved_master;
>>> Imho this shouldn'e belong here but in drm_new_set_master() - i.e. it
>>> should unwind things on error. Similarly, although we restore the old
>>> drm_master (below), we still have is_master, allowed_master and
>>> authenticated set. Thus one can reuse the elevated credentials (is
>>> this the right terminology?) despite that the ioctl has failed.
>>>
>>>> +               else
>>>> +                       drm_master_put(&saved_master);
>>>> +
>>>> +               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..4b5c11c 100644
>>>> --- a/drivers/gpu/drm/drm_fops.c
>>>> +++ b/drivers/gpu/drm/drm_fops.c
>>>> @@ -126,6 +126,56 @@ 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)
>>>> +{
>>>> +       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;
>>>> +
>>>> +       fpriv->is_master = 1;
>>>> +       fpriv->allowed_master = 1;
>>>> +
>>>> +       /* take another reference for the copy in the local file priv */
>>>> +       fpriv->master = drm_master_get(fpriv->minor->master);
>>>> +
>>>> +       fpriv->authenticated = 1;
>>>> +
>>>> +       if (dev->driver->master_create) {
>>>> +               ret = dev->driver->master_create(dev, fpriv->master);
>>>> +               if (ret) {
>>>> +                       /* drop both references if this fails */
>>>> +                       drm_master_put(&fpriv->minor->master);
>>>> +                       drm_master_put(&fpriv->master);
>>>> +                       return ret;
>>>> +               }
>>>> +       }
>>>> +       if (dev->driver->master_set) {
>>>> +               ret = dev->driver->master_set(dev, fpriv, true);
>>>> +               if (ret) {
>>> Afaics both of these callbacks are available from legacy(UMS) drivers
>>> aren't they ? With the radeon UMS removal patches in the works, this
>>> leaves vmwgfx.
>>>
>>> Either way, perhaps we should set is_master, allowed_master and
>>> authenticated after these two ? Or alternatively restore the original
>>> values on error.
>>>
>>> Did I miss something or the above sounds about right ?
>> It does. I'll address this and resend.
> Just wanted to pull this in and noticed there's still this open. New
> version incoming soon?
>
> Thanks, Daniel
OK. Sent out v2. I'd prefer if this could go through -fixes and I've
also cc'd stable since it fixes a real kernel crash on vmwgfx.

Thomas

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