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.
elsedrm_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