| Message ID | 1448887461-3175-1-git-send-email-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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); > }
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); >> } >
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
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
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
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
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
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
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
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
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);
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(-)