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@xxxxxxxxxx> 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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel