[PATCH v5 0/3] drm: address potential UAF bugs with drm_master ptrs

[PATCH v5 0/3] drm: address potential UAF bugs with drm_master ptrs

[Desmond Cheong Zhi Xi]

This patch series addresses potential use-after-free errors when dereferencing pointers to struct drm_master. These were identified after one such bug was caught by Syzbot in drm_getunique():
https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803

The series is broken up into three patches:

1. Move a call to drm_is_current_master() out from a section locked by &dev->mode_config.mutex in drm_mode_getconnector(). This patch does not apply to stable.

2. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.

3. Identify areas in drm_lease.c where pointers to struct drm_master are dereferenced, and ensure that the master pointers are not freed during use.

Changes in v4 -> v5:
- Patch 1:
Add patch 1 to the series. The changes in patch 1 do not apply to stable because they apply to new changes in the drm-misc-next branch. This patch moves the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex.

Additionally, added a missing semicolon to the patch, caught by the intel-gfx CI.

- Patch 2:
Move changes to drm_connector.c into patch 1.

Changes in v3 -> v4:
- Patch 2:
Move the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. As suggested by Daniel Vetter. This avoids a circular lock lock dependency as reported here https://patchwork.freedesktop.org/patch/440406/

Additionally, inside drm_is_current_master, instead of grabbing &fpriv->master->dev->master_mutex, we grab &fpriv->minor->dev->master_mutex to avoid dereferencing a null ptr if fpriv->master is not set.

- Patch 3:
Modify kerneldoc formatting.

Additionally, add a file_priv->master NULL check inside drm_file_get_master, and handle the NULL result accordingly in drm_lease.c. As suggested by Daniel Vetter.

Changes in v2 -> v3:
- Patch 2:
Move the definition of drm_is_current_master and the _locked version higher up in drm_auth.c to avoid needing a forward declaration of drm_is_current_master_locked. As suggested by Daniel Vetter.

- Patch 3:
Instead of leaking drm_device.master_mutex into drm_lease.c to protect drm_master pointers, add a new drm_file_get_master() function that returns drm_file->master while increasing its reference count, to prevent drm_file->master from being freed. As suggested by Daniel Vetter.

Changes in v1 -> v2:
- Patch 3:
Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov.

Desmond Cheong Zhi Xi (3):
  drm: avoid circular locks in drm_mode_getconnector
  drm: add a locked version of drm_is_current_master
  drm: protect drm_master pointers in drm_lease.c

 drivers/gpu/drm/drm_auth.c      | 76 ++++++++++++++++++++++++--------
 drivers/gpu/drm/drm_connector.c |  5 ++-
 drivers/gpu/drm/drm_lease.c     | 77 ++++++++++++++++++++++++---------
 include/drm/drm_auth.h          |  1 +
 include/drm/drm_file.h          | 15 +++++--
 5 files changed, 131 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH v5 1/3] drm: avoid circular locks in drm_mode_getconnector

[Desmond Cheong Zhi Xi]

In preparation for a future patch to take a lock on
drm_device.master_mutex inside drm_is_current_master(), we first move
the call to drm_is_current_master() in drm_mode_getconnector out from the
section locked by &dev->mode_config.mutex. This avoids creating a
circular lock dependency.

Failing to avoid this lock dependency produces the following lockdep
splat:

======================================================
WARNING: possible circular locking dependency detected
5.13.0-rc7-CI-CI_DRM_10254+ #1 Not tainted
------------------------------------------------------
kms_frontbuffer/1087 is trying to acquire lock:
ffff88810dcd01a8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_is_current_master+0x1b/0x40
but task is already holding lock:
ffff88810dcd0488 (&dev->mode_config.mutex){+.+.}-{3:3}, at: drm_mode_getconnector+0x1c6/0x4a0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (&dev->mode_config.mutex){+.+.}-{3:3}:
       __mutex_lock+0xab/0x970
       drm_client_modeset_probe+0x22e/0xca0
       __drm_fb_helper_initial_config_and_unlock+0x42/0x540
       intel_fbdev_initial_config+0xf/0x20 [i915]
       async_run_entry_fn+0x28/0x130
       process_one_work+0x26d/0x5c0
       worker_thread+0x37/0x380
       kthread+0x144/0x170
       ret_from_fork+0x1f/0x30
-> #1 (&client->modeset_mutex){+.+.}-{3:3}:
       __mutex_lock+0xab/0x970
       drm_client_modeset_commit_locked+0x1c/0x180
       drm_client_modeset_commit+0x1c/0x40
       __drm_fb_helper_restore_fbdev_mode_unlocked+0x88/0xb0
       drm_fb_helper_set_par+0x34/0x40
       intel_fbdev_set_par+0x11/0x40 [i915]
       fbcon_init+0x270/0x4f0
       visual_init+0xc6/0x130
       do_bind_con_driver+0x1e5/0x2d0
       do_take_over_console+0x10e/0x180
       do_fbcon_takeover+0x53/0xb0
       register_framebuffer+0x22d/0x310
       __drm_fb_helper_initial_config_and_unlock+0x36c/0x540
       intel_fbdev_initial_config+0xf/0x20 [i915]
       async_run_entry_fn+0x28/0x130
       process_one_work+0x26d/0x5c0
       worker_thread+0x37/0x380
       kthread+0x144/0x170
       ret_from_fork+0x1f/0x30
-> #0 (&dev->master_mutex){+.+.}-{3:3}:
       __lock_acquire+0x151e/0x2590
       lock_acquire+0xd1/0x3d0
       __mutex_lock+0xab/0x970
       drm_is_current_master+0x1b/0x40
       drm_mode_getconnector+0x37e/0x4a0
       drm_ioctl_kernel+0xa8/0xf0
       drm_ioctl+0x1e8/0x390
       __x64_sys_ioctl+0x6a/0xa0
       do_syscall_64+0x39/0xb0
       entry_SYSCALL_64_after_hwframe+0x44/0xae
other info that might help us debug this:
Chain exists of: &dev->master_mutex --> &client->modeset_mutex --> &dev->mode_config.mutex
 Possible unsafe locking scenario:
       CPU0                    CPU1
       ----                    ----
  lock(&dev->mode_config.mutex);
                               lock(&client->modeset_mutex);
                               lock(&dev->mode_config.mutex);
  lock(&dev->master_mutex);
*** DEADLOCK ***
1 lock held by kms_frontbuffer/1087:
 #0: ffff88810dcd0488 (&dev->mode_config.mutex){+.+.}-{3:3}, at: drm_mode_getconnector+0x1c6/0x4a0
stack backtrace:
CPU: 7 PID: 1087 Comm: kms_frontbuffer Not tainted 5.13.0-rc7-CI-CI_DRM_10254+ #1
Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750 06/14/2019
Call Trace:
 dump_stack+0x7f/0xad
 check_noncircular+0x12e/0x150
 __lock_acquire+0x151e/0x2590
 lock_acquire+0xd1/0x3d0
 __mutex_lock+0xab/0x970
 drm_is_current_master+0x1b/0x40
 drm_mode_getconnector+0x37e/0x4a0
 drm_ioctl_kernel+0xa8/0xf0
 drm_ioctl+0x1e8/0x390
 __x64_sys_ioctl+0x6a/0xa0
 do_syscall_64+0x39/0xb0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_connector.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index da39e7ff6965..2ba257b1ae20 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2414,6 +2414,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	struct drm_mode_modeinfo u_mode;
 	struct drm_mode_modeinfo __user *mode_ptr;
 	uint32_t __user *encoder_ptr;
+	bool is_current_master;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
@@ -2444,9 +2445,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	out_resp->connector_type = connector->connector_type;
 	out_resp->connector_type_id = connector->connector_type_id;
 
+	is_current_master = drm_is_current_master(file_priv);
+
 	mutex_lock(&dev->mode_config.mutex);
 	if (out_resp->count_modes == 0) {
-		if (drm_is_current_master(file_priv))
+		if (is_current_master)
 			connector->funcs->fill_modes(connector,
 						     dev->mode_config.max_width,
 						     dev->mode_config.max_height);
-- 
2.25.1

[PATCH v5 2/3] drm: add a locked version of drm_is_current_master

[Desmond Cheong Zhi Xi]

While checking the master status of the DRM file in
drm_is_current_master(), the device's master mutex should be
held. Without the mutex, the pointer fpriv->master may be freed
concurrently by another process calling drm_setmaster_ioctl(). This
could lead to use-after-free errors when the pointer is subsequently
dereferenced in drm_lease_owner().

The callers of drm_is_current_master() from drm_auth.c hold the
device's master mutex, but external callers do not. Hence, we implement
drm_is_current_master_locked() to be used within drm_auth.c, and
modify drm_is_current_master() to grab the device's master mutex
before checking the master status.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_auth.c | 51 ++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index f00e5abdbbf4..ab1863c5a5a0 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -61,6 +61,35 @@
  * trusted clients.
  */
 
+static bool drm_is_current_master_locked(struct drm_file *fpriv)
+{
+	lockdep_assert_held_once(&fpriv->minor->dev->master_mutex);
+
+	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
+}
+
+/**
+ * drm_is_current_master - checks whether @priv is the current master
+ * @fpriv: DRM file private
+ *
+ * Checks whether @fpriv is current master on its device. This decides whether a
+ * client is allowed to run DRM_MASTER IOCTLs.
+ *
+ * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
+ * - the current master is assumed to own the non-shareable display hardware.
+ */
+bool drm_is_current_master(struct drm_file *fpriv)
+{
+	bool ret;
+
+	mutex_lock(&fpriv->minor->dev->master_mutex);
+	ret = drm_is_current_master_locked(fpriv);
+	mutex_unlock(&fpriv->minor->dev->master_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_is_current_master);
+
 int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
 	struct drm_auth *auth = data;
@@ -223,7 +252,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto out_unlock;
 
-	if (drm_is_current_master(file_priv))
+	if (drm_is_current_master_locked(file_priv))
 		goto out_unlock;
 
 	if (dev->master) {
@@ -272,7 +301,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto out_unlock;
 
-	if (!drm_is_current_master(file_priv)) {
+	if (!drm_is_current_master_locked(file_priv)) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -321,7 +350,7 @@ void drm_master_release(struct drm_file *file_priv)
 	if (file_priv->magic)
 		idr_remove(&file_priv->master->magic_map, file_priv->magic);
 
-	if (!drm_is_current_master(file_priv))
+	if (!drm_is_current_master_locked(file_priv))
 		goto out;
 
 	drm_legacy_lock_master_cleanup(dev, master);
@@ -342,22 +371,6 @@ void drm_master_release(struct drm_file *file_priv)
 	mutex_unlock(&dev->master_mutex);
 }
 
-/**
- * drm_is_current_master - checks whether @priv is the current master
- * @fpriv: DRM file private
- *
- * Checks whether @fpriv is current master on its device. This decides whether a
- * client is allowed to run DRM_MASTER IOCTLs.
- *
- * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
- * - the current master is assumed to own the non-shareable display hardware.
- */
-bool drm_is_current_master(struct drm_file *fpriv)
-{
-	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
-}
-EXPORT_SYMBOL(drm_is_current_master);
-
 /**
  * drm_master_get - reference a master pointer
  * @master: &struct drm_master
-- 
2.25.1

[PATCH v5 3/3] drm: protect drm_master pointers in drm_lease.c

[Desmond Cheong Zhi Xi]

Currently, direct copies of drm_file->master pointers should be
protected by drm_device.master_mutex when being dereferenced. This is
because drm_file->master is not invariant for the lifetime of
drm_file. If drm_file is not the creator of master, then
drm_file->is_master is false, and a call to drm_setmaster_ioctl will
invoke drm_new_set_master, which then allocates a new master for
drm_file and puts the old master.

Thus, without holding drm_device.master_mutex, the old value of
drm_file->master could be freed while it is being used by another
concurrent process.

In drm_lease.c, there are multiple instances where drm_file->master is
accessed and dereferenced while drm_device.master_mutex is not
held. This makes drm_lease.c vulnerable to use-after-free bugs.

We address this issue in 3 ways:

1. Clarify in the kerneldoc that drm_file->master is protected by
drm_device.master_mutex.

2. Add a new drm_file_get_master() function that calls drm_master_get
on drm_file->master while holding on to drm_device.master_mutex. Since
drm_master_get increments the reference count of master, this
prevents master from being freed until we unreference it with
drm_master_put.

3. In each case where drm_file->master is directly accessed and
eventually dereferenced in drm_lease.c, we wrap the access in a call
to the new drm_file_get_master function, then unreference the master
pointer once we are done using it.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_auth.c  | 25 ++++++++++++
 drivers/gpu/drm/drm_lease.c | 77 +++++++++++++++++++++++++++----------
 include/drm/drm_auth.h      |  1 +
 include/drm/drm_file.h      | 15 ++++++--
 4 files changed, 95 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index ab1863c5a5a0..c36a0b72be26 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -384,6 +384,31 @@ struct drm_master *drm_master_get(struct drm_master *master)
 }
 EXPORT_SYMBOL(drm_master_get);
 
+/**
+ * drm_file_get_master - reference &drm_file.master of @file_priv
+ * @file_priv: DRM file private
+ *
+ * Increments the reference count of @file_priv's &drm_file.master and returns
+ * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL.
+ *
+ * Master pointers returned from this function should be unreferenced using
+ * drm_master_put().
+ */
+struct drm_master *drm_file_get_master(struct drm_file *file_priv)
+{
+	struct drm_master *master = NULL;
+
+	mutex_lock(&file_priv->minor->dev->master_mutex);
+	if (!file_priv->master)
+		goto unlock;
+	master = drm_master_get(file_priv->master);
+
+unlock:
+	mutex_unlock(&file_priv->minor->dev->master_mutex);
+	return master;
+}
+EXPORT_SYMBOL(drm_file_get_master);
+
 static void drm_master_destroy(struct kref *kref)
 {
 	struct drm_master *master = container_of(kref, struct drm_master, refcount);
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 00fb433bcef1..cdcc87fa9685 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id)
  */
 bool _drm_lease_held(struct drm_file *file_priv, int id)
 {
-	if (!file_priv || !file_priv->master)
+	bool ret;
+	struct drm_master *master;
+
+	if (!file_priv)
 		return true;
 
-	return _drm_lease_held_master(file_priv->master, id);
+	master = drm_file_get_master(file_priv);
+	if (master == NULL)
+		return true;
+	ret = _drm_lease_held_master(master, id);
+	drm_master_put(&master);
+
+	return ret;
 }
 
 /**
@@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
 	struct drm_master *master;
 	bool ret;
 
-	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
+	if (!file_priv)
 		return true;
 
-	master = file_priv->master;
+	master = drm_file_get_master(file_priv);
+	if (master == NULL)
+		return true;
+	if (!master->lessor) {
+		drm_master_put(&master);
+		return true;
+	}
 	mutex_lock(&master->dev->mode_config.idr_mutex);
 	ret = _drm_lease_held_master(master, id);
 	mutex_unlock(&master->dev->mode_config.idr_mutex);
+	drm_master_put(&master);
 	return ret;
 }
 
@@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
 	int count_in, count_out;
 	uint32_t crtcs_out = 0;
 
-	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
+	if (!file_priv)
 		return crtcs_in;
 
-	master = file_priv->master;
+	master = drm_file_get_master(file_priv);
+	if (master == NULL)
+		return crtcs_in;
+	if (!master->lessor) {
+		drm_master_put(&master);
+		return crtcs_in;
+	}
 	dev = master->dev;
 
 	count_in = count_out = 0;
@@ -176,6 +198,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
 		count_in++;
 	}
 	mutex_unlock(&master->dev->mode_config.idr_mutex);
+	drm_master_put(&master);
 	return crtcs_out;
 }
 
@@ -489,7 +512,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	size_t object_count;
 	int ret = 0;
 	struct idr leases;
-	struct drm_master *lessor = lessor_priv->master;
+	struct drm_master *lessor;
 	struct drm_master *lessee = NULL;
 	struct file *lessee_file = NULL;
 	struct file *lessor_file = lessor_priv->filp;
@@ -501,12 +524,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	/* Do not allow sub-leases */
-	if (lessor->lessor) {
-		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
-		return -EINVAL;
-	}
-
 	/* need some objects */
 	if (cl->object_count == 0) {
 		DRM_DEBUG_LEASE("no objects in lease\n");
@@ -518,12 +535,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	lessor = drm_file_get_master(lessor_priv);
+	/* Do not allow sub-leases */
+	if (lessor->lessor) {
+		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
+		ret = -EINVAL;
+		goto out_lessor;
+	}
+
 	object_count = cl->object_count;
 
 	object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
 			array_size(object_count, sizeof(__u32)));
-	if (IS_ERR(object_ids))
-		return PTR_ERR(object_ids);
+	if (IS_ERR(object_ids)) {
+		ret = PTR_ERR(object_ids);
+		goto out_lessor;
+	}
 
 	idr_init(&leases);
 
@@ -534,14 +561,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	if (ret) {
 		DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
 		idr_destroy(&leases);
-		return ret;
+		goto out_lessor;
 	}
 
 	/* Allocate a file descriptor for the lease */
 	fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
 	if (fd < 0) {
 		idr_destroy(&leases);
-		return fd;
+		ret = fd;
+		goto out_lessor;
 	}
 
 	DRM_DEBUG_LEASE("Creating lease\n");
@@ -577,6 +605,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	/* Hook up the fd */
 	fd_install(fd, lessee_file);
 
+	drm_master_put(&lessor);
 	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
 	return 0;
 
@@ -586,6 +615,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 out_leases:
 	put_unused_fd(fd);
 
+out_lessor:
+	drm_master_put(&lessor);
 	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
 	return ret;
 }
@@ -608,7 +639,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
 	struct drm_mode_list_lessees *arg = data;
 	__u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr);
 	__u32 count_lessees = arg->count_lessees;
-	struct drm_master *lessor = lessor_priv->master, *lessee;
+	struct drm_master *lessor, *lessee;
 	int count;
 	int ret = 0;
 
@@ -619,6 +650,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
+	lessor = drm_file_get_master(lessor_priv);
 	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
 
 	mutex_lock(&dev->mode_config.idr_mutex);
@@ -642,6 +674,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
 		arg->count_lessees = count;
 
 	mutex_unlock(&dev->mode_config.idr_mutex);
+	drm_master_put(&lessor);
 
 	return ret;
 }
@@ -661,7 +694,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
 	struct drm_mode_get_lease *arg = data;
 	__u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr);
 	__u32 count_objects = arg->count_objects;
-	struct drm_master *lessee = lessee_priv->master;
+	struct drm_master *lessee;
 	struct idr *object_idr;
 	int count;
 	void *entry;
@@ -675,6 +708,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
+	lessee = drm_file_get_master(lessee_priv);
 	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
 
 	mutex_lock(&dev->mode_config.idr_mutex);
@@ -702,6 +736,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
 		arg->count_objects = count;
 
 	mutex_unlock(&dev->mode_config.idr_mutex);
+	drm_master_put(&lessee);
 
 	return ret;
 }
@@ -720,7 +755,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 				void *data, struct drm_file *lessor_priv)
 {
 	struct drm_mode_revoke_lease *arg = data;
-	struct drm_master *lessor = lessor_priv->master;
+	struct drm_master *lessor;
 	struct drm_master *lessee;
 	int ret = 0;
 
@@ -730,6 +765,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
+	lessor = drm_file_get_master(lessor_priv);
 	mutex_lock(&dev->mode_config.idr_mutex);
 
 	lessee = _drm_find_lessee(lessor, arg->lessee_id);
@@ -750,6 +786,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 
 fail:
 	mutex_unlock(&dev->mode_config.idr_mutex);
+	drm_master_put(&lessor);
 
 	return ret;
 }
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
index 6bf8b2b78991..f99d3417f304 100644
--- a/include/drm/drm_auth.h
+++ b/include/drm/drm_auth.h
@@ -107,6 +107,7 @@ struct drm_master {
 };
 
 struct drm_master *drm_master_get(struct drm_master *master);
+struct drm_master *drm_file_get_master(struct drm_file *file_priv);
 void drm_master_put(struct drm_master **master);
 bool drm_is_current_master(struct drm_file *fpriv);
 
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index b81b3bfb08c8..e9931fca4ab7 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -226,9 +226,18 @@ struct drm_file {
 	/**
 	 * @master:
 	 *
-	 * Master this node is currently associated with. Only relevant if
-	 * drm_is_primary_client() returns true. Note that this only
-	 * matches &drm_device.master if the master is the currently active one.
+	 * Master this node is currently associated with. Protected by struct
+	 * &drm_device.master_mutex.
+	 *
+	 * Only relevant if drm_is_primary_client() returns true. Note that
+	 * this only matches &drm_device.master if the master is the currently
+	 * active one.
+	 *
+	 * When obtaining a copy of this pointer, it is recommended to either
+	 * hold struct &drm_device.master_mutex for the duration of the
+	 * pointer's use, or to use drm_file_get_master() if struct
+	 * &drm_device.master_mutex is not currently held and there is no other
+	 * need to hold it. This prevents @master from being freed during use.
 	 *
 	 * See also @authentication and @is_master and the :ref:`section on
 	 * primary nodes and authentication <drm_primary_node>`.
-- 
2.25.1

Re: [PATCH v5 3/3] drm: protect drm_master pointers in drm_lease.c

[Daniel Vetter]

On Tue, Jun 29, 2021 at 11:37:06AM +0800, Desmond Cheong Zhi Xi wrote:
> Currently, direct copies of drm_file->master pointers should be
> protected by drm_device.master_mutex when being dereferenced. This is
> because drm_file->master is not invariant for the lifetime of
> drm_file. If drm_file is not the creator of master, then
> drm_file->is_master is false, and a call to drm_setmaster_ioctl will
> invoke drm_new_set_master, which then allocates a new master for
> drm_file and puts the old master.
> 
> Thus, without holding drm_device.master_mutex, the old value of
> drm_file->master could be freed while it is being used by another
> concurrent process.
> 
> In drm_lease.c, there are multiple instances where drm_file->master is
> accessed and dereferenced while drm_device.master_mutex is not
> held. This makes drm_lease.c vulnerable to use-after-free bugs.
> 
> We address this issue in 3 ways:
> 
> 1. Clarify in the kerneldoc that drm_file->master is protected by
> drm_device.master_mutex.
> 
> 2. Add a new drm_file_get_master() function that calls drm_master_get
> on drm_file->master while holding on to drm_device.master_mutex. Since
> drm_master_get increments the reference count of master, this
> prevents master from being freed until we unreference it with
> drm_master_put.
> 
> 3. In each case where drm_file->master is directly accessed and
> eventually dereferenced in drm_lease.c, we wrap the access in a call
> to the new drm_file_get_master function, then unreference the master
> pointer once we are done using it.
> 
> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>

Series looks very nice, let's see what intel-gfx-ci says. You should get a
mail, but results are also here:

https://patchwork.freedesktop.org/series/91969/#rev2

One tiny comment below.

> ---
>  drivers/gpu/drm/drm_auth.c  | 25 ++++++++++++
>  drivers/gpu/drm/drm_lease.c | 77 +++++++++++++++++++++++++++----------
>  include/drm/drm_auth.h      |  1 +
>  include/drm/drm_file.h      | 15 ++++++--
>  4 files changed, 95 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index ab1863c5a5a0..c36a0b72be26 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -384,6 +384,31 @@ struct drm_master *drm_master_get(struct drm_master *master)
>  }
>  EXPORT_SYMBOL(drm_master_get);
>  
> +/**
> + * drm_file_get_master - reference &drm_file.master of @file_priv
> + * @file_priv: DRM file private
> + *
> + * Increments the reference count of @file_priv's &drm_file.master and returns
> + * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL.
> + *
> + * Master pointers returned from this function should be unreferenced using
> + * drm_master_put().
> + */
> +struct drm_master *drm_file_get_master(struct drm_file *file_priv)
> +{
> +	struct drm_master *master = NULL;
> +
> +	mutex_lock(&file_priv->minor->dev->master_mutex);
> +	if (!file_priv->master)
> +		goto unlock;
> +	master = drm_master_get(file_priv->master);
> +
> +unlock:
> +	mutex_unlock(&file_priv->minor->dev->master_mutex);
> +	return master;
> +}
> +EXPORT_SYMBOL(drm_file_get_master);
> +
>  static void drm_master_destroy(struct kref *kref)
>  {
>  	struct drm_master *master = container_of(kref, struct drm_master, refcount);
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 00fb433bcef1..cdcc87fa9685 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id)
>   */
>  bool _drm_lease_held(struct drm_file *file_priv, int id)
>  {
> -	if (!file_priv || !file_priv->master)
> +	bool ret;
> +	struct drm_master *master;
> +
> +	if (!file_priv)
>  		return true;
>  
> -	return _drm_lease_held_master(file_priv->master, id);
> +	master = drm_file_get_master(file_priv);
> +	if (master == NULL)
> +		return true;
> +	ret = _drm_lease_held_master(master, id);
> +	drm_master_put(&master);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>  	struct drm_master *master;
>  	bool ret;
>  
> -	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> +	if (!file_priv)
>  		return true;
>  
> -	master = file_priv->master;
> +	master = drm_file_get_master(file_priv);
> +	if (master == NULL)
> +		return true;
> +	if (!master->lessor) {
> +		drm_master_put(&master);
> +		return true;
> +	}
>  	mutex_lock(&master->dev->mode_config.idr_mutex);
>  	ret = _drm_lease_held_master(master, id);
>  	mutex_unlock(&master->dev->mode_config.idr_mutex);
> +	drm_master_put(&master);
>  	return ret;
>  }
>  
> @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>  	int count_in, count_out;
>  	uint32_t crtcs_out = 0;
>  
> -	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> +	if (!file_priv)
>  		return crtcs_in;
>  
> -	master = file_priv->master;
> +	master = drm_file_get_master(file_priv);
> +	if (master == NULL)
> +		return crtcs_in;
> +	if (!master->lessor) {
> +		drm_master_put(&master);
> +		return crtcs_in;
> +	}
>  	dev = master->dev;
>  
>  	count_in = count_out = 0;
> @@ -176,6 +198,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>  		count_in++;
>  	}
>  	mutex_unlock(&master->dev->mode_config.idr_mutex);
> +	drm_master_put(&master);
>  	return crtcs_out;
>  }
>  
> @@ -489,7 +512,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	size_t object_count;
>  	int ret = 0;
>  	struct idr leases;
> -	struct drm_master *lessor = lessor_priv->master;
> +	struct drm_master *lessor;
>  	struct drm_master *lessee = NULL;
>  	struct file *lessee_file = NULL;
>  	struct file *lessor_file = lessor_priv->filp;
> @@ -501,12 +524,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> -	/* Do not allow sub-leases */
> -	if (lessor->lessor) {
> -		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
> -		return -EINVAL;
> -	}
> -
>  	/* need some objects */
>  	if (cl->object_count == 0) {
>  		DRM_DEBUG_LEASE("no objects in lease\n");
> @@ -518,12 +535,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	lessor = drm_file_get_master(lessor_priv);
> +	/* Do not allow sub-leases */
> +	if (lessor->lessor) {
> +		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
> +		ret = -EINVAL;
> +		goto out_lessor;
> +	}
> +
>  	object_count = cl->object_count;
>  
>  	object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
>  			array_size(object_count, sizeof(__u32)));
> -	if (IS_ERR(object_ids))
> -		return PTR_ERR(object_ids);
> +	if (IS_ERR(object_ids)) {
> +		ret = PTR_ERR(object_ids);
> +		goto out_lessor;
> +	}
>  
>  	idr_init(&leases);
>  
> @@ -534,14 +561,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	if (ret) {
>  		DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
>  		idr_destroy(&leases);
> -		return ret;
> +		goto out_lessor;
>  	}
>  
>  	/* Allocate a file descriptor for the lease */
>  	fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
>  	if (fd < 0) {
>  		idr_destroy(&leases);
> -		return fd;
> +		ret = fd;
> +		goto out_lessor;
>  	}
>  
>  	DRM_DEBUG_LEASE("Creating lease\n");
> @@ -577,6 +605,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	/* Hook up the fd */
>  	fd_install(fd, lessee_file);
>  
> +	drm_master_put(&lessor);
>  	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>  	return 0;
>  
> @@ -586,6 +615,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  out_leases:
>  	put_unused_fd(fd);
>  
> +out_lessor:
> +	drm_master_put(&lessor);
>  	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
>  	return ret;
>  }
> @@ -608,7 +639,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>  	struct drm_mode_list_lessees *arg = data;
>  	__u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr);
>  	__u32 count_lessees = arg->count_lessees;
> -	struct drm_master *lessor = lessor_priv->master, *lessee;
> +	struct drm_master *lessor, *lessee;
>  	int count;
>  	int ret = 0;
>  
> @@ -619,6 +650,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> +	lessor = drm_file_get_master(lessor_priv);
>  	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
>  
>  	mutex_lock(&dev->mode_config.idr_mutex);
> @@ -642,6 +674,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>  		arg->count_lessees = count;
>  
>  	mutex_unlock(&dev->mode_config.idr_mutex);
> +	drm_master_put(&lessor);
>  
>  	return ret;
>  }
> @@ -661,7 +694,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>  	struct drm_mode_get_lease *arg = data;
>  	__u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr);
>  	__u32 count_objects = arg->count_objects;
> -	struct drm_master *lessee = lessee_priv->master;
> +	struct drm_master *lessee;
>  	struct idr *object_idr;
>  	int count;
>  	void *entry;
> @@ -675,6 +708,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> +	lessee = drm_file_get_master(lessee_priv);
>  	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
>  
>  	mutex_lock(&dev->mode_config.idr_mutex);
> @@ -702,6 +736,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>  		arg->count_objects = count;
>  
>  	mutex_unlock(&dev->mode_config.idr_mutex);
> +	drm_master_put(&lessee);
>  
>  	return ret;
>  }
> @@ -720,7 +755,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>  				void *data, struct drm_file *lessor_priv)
>  {
>  	struct drm_mode_revoke_lease *arg = data;
> -	struct drm_master *lessor = lessor_priv->master;
> +	struct drm_master *lessor;
>  	struct drm_master *lessee;
>  	int ret = 0;
>  
> @@ -730,6 +765,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> +	lessor = drm_file_get_master(lessor_priv);
>  	mutex_lock(&dev->mode_config.idr_mutex);
>  
>  	lessee = _drm_find_lessee(lessor, arg->lessee_id);
> @@ -750,6 +786,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>  
>  fail:
>  	mutex_unlock(&dev->mode_config.idr_mutex);
> +	drm_master_put(&lessor);
>  
>  	return ret;
>  }
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index 6bf8b2b78991..f99d3417f304 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -107,6 +107,7 @@ struct drm_master {
>  };
>  
>  struct drm_master *drm_master_get(struct drm_master *master);
> +struct drm_master *drm_file_get_master(struct drm_file *file_priv);
>  void drm_master_put(struct drm_master **master);
>  bool drm_is_current_master(struct drm_file *fpriv);
>  
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index b81b3bfb08c8..e9931fca4ab7 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -226,9 +226,18 @@ struct drm_file {
>  	/**
>  	 * @master:
>  	 *
> -	 * Master this node is currently associated with. Only relevant if
> -	 * drm_is_primary_client() returns true. Note that this only
> -	 * matches &drm_device.master if the master is the currently active one.
> +	 * Master this node is currently associated with. Protected by struct
> +	 * &drm_device.master_mutex.
> +	 *
> +	 * Only relevant if drm_is_primary_client() returns true. Note that
> +	 * this only matches &drm_device.master if the master is the currently
> +	 * active one.
> +	 *
> +	 * When obtaining a copy of this pointer, it is recommended to either

I found this a bit confusing, since I generally don't think of
dereferencing the pointer as "taking a copy". That's more for the entire
datastructure when you have a memcpy() call, or kmemdup() or something
like that. Also "it is recommended" is a bit weak if you get a
use-after-free if you dont :-)

So instead "When dererencing this pointer either hold ... or use
drm_file_get_master() ...."

Cheers, Daniel

> +	 * hold struct &drm_device.master_mutex for the duration of the
> +	 * pointer's use, or to use drm_file_get_master() if struct
> +	 * &drm_device.master_mutex is not currently held and there is no other
> +	 * need to hold it. This prevents @master from being freed during use.
>  	 *
>  	 * See also @authentication and @is_master and the :ref:`section on
>  	 * primary nodes and authentication <drm_primary_node>`.
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 3/3] drm: protect drm_master pointers in drm_lease.c

[Desmond Cheong Zhi Xi]

On 30/6/21 12:07 am, Daniel Vetter wrote:
> On Tue, Jun 29, 2021 at 11:37:06AM +0800, Desmond Cheong Zhi Xi wrote:
>> Currently, direct copies of drm_file->master pointers should be
>> protected by drm_device.master_mutex when being dereferenced. This is
>> because drm_file->master is not invariant for the lifetime of
>> drm_file. If drm_file is not the creator of master, then
>> drm_file->is_master is false, and a call to drm_setmaster_ioctl will
>> invoke drm_new_set_master, which then allocates a new master for
>> drm_file and puts the old master.
>>
>> Thus, without holding drm_device.master_mutex, the old value of
>> drm_file->master could be freed while it is being used by another
>> concurrent process.
>>
>> In drm_lease.c, there are multiple instances where drm_file->master is
>> accessed and dereferenced while drm_device.master_mutex is not
>> held. This makes drm_lease.c vulnerable to use-after-free bugs.
>>
>> We address this issue in 3 ways:
>>
>> 1. Clarify in the kerneldoc that drm_file->master is protected by
>> drm_device.master_mutex.
>>
>> 2. Add a new drm_file_get_master() function that calls drm_master_get
>> on drm_file->master while holding on to drm_device.master_mutex. Since
>> drm_master_get increments the reference count of master, this
>> prevents master from being freed until we unreference it with
>> drm_master_put.
>>
>> 3. In each case where drm_file->master is directly accessed and
>> eventually dereferenced in drm_lease.c, we wrap the access in a call
>> to the new drm_file_get_master function, then unreference the master
>> pointer once we are done using it.
>>
>> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> 
> Series looks very nice, let's see what intel-gfx-ci says. You should get a
> mail, but results are also here:
> 
> https://patchwork.freedesktop.org/series/91969/#rev2
> 
> One tiny comment below.
> 
>> ---
>>   drivers/gpu/drm/drm_auth.c  | 25 ++++++++++++
>>   drivers/gpu/drm/drm_lease.c | 77 +++++++++++++++++++++++++++----------
>>   include/drm/drm_auth.h      |  1 +
>>   include/drm/drm_file.h      | 15 ++++++--
>>   4 files changed, 95 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>> index ab1863c5a5a0..c36a0b72be26 100644
>> --- a/drivers/gpu/drm/drm_auth.c
>> +++ b/drivers/gpu/drm/drm_auth.c
>> @@ -384,6 +384,31 @@ struct drm_master *drm_master_get(struct drm_master *master)
>>   }
>>   EXPORT_SYMBOL(drm_master_get);
>>   
>> +/**
>> + * drm_file_get_master - reference &drm_file.master of @file_priv
>> + * @file_priv: DRM file private
>> + *
>> + * Increments the reference count of @file_priv's &drm_file.master and returns
>> + * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL.
>> + *
>> + * Master pointers returned from this function should be unreferenced using
>> + * drm_master_put().
>> + */
>> +struct drm_master *drm_file_get_master(struct drm_file *file_priv)
>> +{
>> +	struct drm_master *master = NULL;
>> +
>> +	mutex_lock(&file_priv->minor->dev->master_mutex);
>> +	if (!file_priv->master)
>> +		goto unlock;
>> +	master = drm_master_get(file_priv->master);
>> +
>> +unlock:
>> +	mutex_unlock(&file_priv->minor->dev->master_mutex);
>> +	return master;
>> +}
>> +EXPORT_SYMBOL(drm_file_get_master);
>> +
>>   static void drm_master_destroy(struct kref *kref)
>>   {
>>   	struct drm_master *master = container_of(kref, struct drm_master, refcount);
>> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
>> index 00fb433bcef1..cdcc87fa9685 100644
>> --- a/drivers/gpu/drm/drm_lease.c
>> +++ b/drivers/gpu/drm/drm_lease.c
>> @@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id)
>>    */
>>   bool _drm_lease_held(struct drm_file *file_priv, int id)
>>   {
>> -	if (!file_priv || !file_priv->master)
>> +	bool ret;
>> +	struct drm_master *master;
>> +
>> +	if (!file_priv)
>>   		return true;
>>   
>> -	return _drm_lease_held_master(file_priv->master, id);
>> +	master = drm_file_get_master(file_priv);
>> +	if (master == NULL)
>> +		return true;
>> +	ret = _drm_lease_held_master(master, id);
>> +	drm_master_put(&master);
>> +
>> +	return ret;
>>   }
>>   
>>   /**
>> @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>>   	struct drm_master *master;
>>   	bool ret;
>>   
>> -	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>> +	if (!file_priv)
>>   		return true;
>>   
>> -	master = file_priv->master;
>> +	master = drm_file_get_master(file_priv);
>> +	if (master == NULL)
>> +		return true;
>> +	if (!master->lessor) {
>> +		drm_master_put(&master);
>> +		return true;
>> +	}
>>   	mutex_lock(&master->dev->mode_config.idr_mutex);
>>   	ret = _drm_lease_held_master(master, id);
>>   	mutex_unlock(&master->dev->mode_config.idr_mutex);
>> +	drm_master_put(&master);
>>   	return ret;
>>   }
>>   
>> @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>>   	int count_in, count_out;
>>   	uint32_t crtcs_out = 0;
>>   
>> -	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>> +	if (!file_priv)
>>   		return crtcs_in;
>>   
>> -	master = file_priv->master;
>> +	master = drm_file_get_master(file_priv);
>> +	if (master == NULL)
>> +		return crtcs_in;
>> +	if (!master->lessor) {
>> +		drm_master_put(&master);
>> +		return crtcs_in;
>> +	}
>>   	dev = master->dev;
>>   
>>   	count_in = count_out = 0;
>> @@ -176,6 +198,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>>   		count_in++;
>>   	}
>>   	mutex_unlock(&master->dev->mode_config.idr_mutex);
>> +	drm_master_put(&master);
>>   	return crtcs_out;
>>   }
>>   
>> @@ -489,7 +512,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   	size_t object_count;
>>   	int ret = 0;
>>   	struct idr leases;
>> -	struct drm_master *lessor = lessor_priv->master;
>> +	struct drm_master *lessor;
>>   	struct drm_master *lessee = NULL;
>>   	struct file *lessee_file = NULL;
>>   	struct file *lessor_file = lessor_priv->filp;
>> @@ -501,12 +524,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> -	/* Do not allow sub-leases */
>> -	if (lessor->lessor) {
>> -		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
>> -		return -EINVAL;
>> -	}
>> -
>>   	/* need some objects */
>>   	if (cl->object_count == 0) {
>>   		DRM_DEBUG_LEASE("no objects in lease\n");
>> @@ -518,12 +535,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   		return -EINVAL;
>>   	}
>>   
>> +	lessor = drm_file_get_master(lessor_priv);
>> +	/* Do not allow sub-leases */
>> +	if (lessor->lessor) {
>> +		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
>> +		ret = -EINVAL;
>> +		goto out_lessor;
>> +	}
>> +
>>   	object_count = cl->object_count;
>>   
>>   	object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
>>   			array_size(object_count, sizeof(__u32)));
>> -	if (IS_ERR(object_ids))
>> -		return PTR_ERR(object_ids);
>> +	if (IS_ERR(object_ids)) {
>> +		ret = PTR_ERR(object_ids);
>> +		goto out_lessor;
>> +	}
>>   
>>   	idr_init(&leases);
>>   
>> @@ -534,14 +561,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   	if (ret) {
>>   		DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
>>   		idr_destroy(&leases);
>> -		return ret;
>> +		goto out_lessor;
>>   	}
>>   
>>   	/* Allocate a file descriptor for the lease */
>>   	fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
>>   	if (fd < 0) {
>>   		idr_destroy(&leases);
>> -		return fd;
>> +		ret = fd;
>> +		goto out_lessor;
>>   	}
>>   
>>   	DRM_DEBUG_LEASE("Creating lease\n");
>> @@ -577,6 +605,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   	/* Hook up the fd */
>>   	fd_install(fd, lessee_file);
>>   
>> +	drm_master_put(&lessor);
>>   	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>>   	return 0;
>>   
>> @@ -586,6 +615,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   out_leases:
>>   	put_unused_fd(fd);
>>   
>> +out_lessor:
>> +	drm_master_put(&lessor);
>>   	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
>>   	return ret;
>>   }
>> @@ -608,7 +639,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>   	struct drm_mode_list_lessees *arg = data;
>>   	__u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr);
>>   	__u32 count_lessees = arg->count_lessees;
>> -	struct drm_master *lessor = lessor_priv->master, *lessee;
>> +	struct drm_master *lessor, *lessee;
>>   	int count;
>>   	int ret = 0;
>>   
>> @@ -619,6 +650,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> +	lessor = drm_file_get_master(lessor_priv);
>>   	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
>>   
>>   	mutex_lock(&dev->mode_config.idr_mutex);
>> @@ -642,6 +674,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>   		arg->count_lessees = count;
>>   
>>   	mutex_unlock(&dev->mode_config.idr_mutex);
>> +	drm_master_put(&lessor);
>>   
>>   	return ret;
>>   }
>> @@ -661,7 +694,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>   	struct drm_mode_get_lease *arg = data;
>>   	__u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr);
>>   	__u32 count_objects = arg->count_objects;
>> -	struct drm_master *lessee = lessee_priv->master;
>> +	struct drm_master *lessee;
>>   	struct idr *object_idr;
>>   	int count;
>>   	void *entry;
>> @@ -675,6 +708,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> +	lessee = drm_file_get_master(lessee_priv);
>>   	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
>>   
>>   	mutex_lock(&dev->mode_config.idr_mutex);
>> @@ -702,6 +736,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>   		arg->count_objects = count;
>>   
>>   	mutex_unlock(&dev->mode_config.idr_mutex);
>> +	drm_master_put(&lessee);
>>   
>>   	return ret;
>>   }
>> @@ -720,7 +755,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>   				void *data, struct drm_file *lessor_priv)
>>   {
>>   	struct drm_mode_revoke_lease *arg = data;
>> -	struct drm_master *lessor = lessor_priv->master;
>> +	struct drm_master *lessor;
>>   	struct drm_master *lessee;
>>   	int ret = 0;
>>   
>> @@ -730,6 +765,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> +	lessor = drm_file_get_master(lessor_priv);
>>   	mutex_lock(&dev->mode_config.idr_mutex);
>>   
>>   	lessee = _drm_find_lessee(lessor, arg->lessee_id);
>> @@ -750,6 +786,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>   
>>   fail:
>>   	mutex_unlock(&dev->mode_config.idr_mutex);
>> +	drm_master_put(&lessor);
>>   
>>   	return ret;
>>   }
>> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
>> index 6bf8b2b78991..f99d3417f304 100644
>> --- a/include/drm/drm_auth.h
>> +++ b/include/drm/drm_auth.h
>> @@ -107,6 +107,7 @@ struct drm_master {
>>   };
>>   
>>   struct drm_master *drm_master_get(struct drm_master *master);
>> +struct drm_master *drm_file_get_master(struct drm_file *file_priv);
>>   void drm_master_put(struct drm_master **master);
>>   bool drm_is_current_master(struct drm_file *fpriv);
>>   
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index b81b3bfb08c8..e9931fca4ab7 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -226,9 +226,18 @@ struct drm_file {
>>   	/**
>>   	 * @master:
>>   	 *
>> -	 * Master this node is currently associated with. Only relevant if
>> -	 * drm_is_primary_client() returns true. Note that this only
>> -	 * matches &drm_device.master if the master is the currently active one.
>> +	 * Master this node is currently associated with. Protected by struct
>> +	 * &drm_device.master_mutex.
>> +	 *
>> +	 * Only relevant if drm_is_primary_client() returns true. Note that
>> +	 * this only matches &drm_device.master if the master is the currently
>> +	 * active one.
>> +	 *
>> +	 * When obtaining a copy of this pointer, it is recommended to either
> 
> I found this a bit confusing, since I generally don't think of
> dereferencing the pointer as "taking a copy". That's more for the entire
> datastructure when you have a memcpy() call, or kmemdup() or something
> like that. Also "it is recommended" is a bit weak if you get a
> use-after-free if you dont :-)
> 
> So instead "When dererencing this pointer either hold ... or use
> drm_file_get_master() ...."
> 
> Cheers, Daniel
> 
>> +	 * hold struct &drm_device.master_mutex for the duration of the
>> +	 * pointer's use, or to use drm_file_get_master() if struct
>> +	 * &drm_device.master_mutex is not currently held and there is no other
>> +	 * need to hold it. This prevents @master from being freed during use.
>>   	 *
>>   	 * See also @authentication and @is_master and the :ref:`section on
>>   	 * primary nodes and authentication <drm_primary_node>`.
>> -- 
>> 2.25.1
>>
> 

Hi Daniel,

Thanks for the suggestion, I'll clarify the kerneldoc accordingly.

Regarding the results from intel-gfx-ci, it seems that the patch is 
inverting the lock hierarchy for
&dev->master_mutex --> &dev->mode_config.idr_mutex

Currently the dmesg warnings share a common call trace:
drm_file_get_master+0x1b/0x70
_drm_lease_held+0x21/0x70
__drm_mode_object_find+0xd1/0xe0

Looking at the functions that lock &dev->mode_config.idr_mutex, this 
should be the only instance of this lock order inversion.

I'm thinking the call to _drm_lease_held can be moved outside of the 
&dev->mode_config.idr_mutex lock in __drm_mode_object. Any thoughts?

diff --git a/drivers/gpu/drm/drm_mode_object.c 
b/drivers/gpu/drm/drm_mode_object.c
index b26588b52795..63d35f1f98dd 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -146,16 +146,18 @@ struct drm_mode_object 
*__drm_mode_object_find(struct drm_device *dev,
         if (obj && obj->id != id)
                 obj = NULL;

-       if (obj && drm_mode_object_lease_required(obj->type) &&
-           !_drm_lease_held(file_priv, obj->id))
-               obj = NULL;
-
         if (obj && obj->free_cb) {
                 if (!kref_get_unless_zero(&obj->refcount))
                         obj = NULL;
         }
         mutex_unlock(&dev->mode_config.idr_mutex);

+       if (obj && drm_mode_object_lease_required(obj->type) &&
+               !_drm_lease_held(file_priv, obj->id)) {
+               drm_mode_object_put(obj);
+               obj = NULL;
+       }
+
         return obj;
  }

Re: [PATCH v5 3/3] drm: protect drm_master pointers in drm_lease.c

[Daniel Vetter]

On Wed, Jun 30, 2021 at 9:18 AM Desmond Cheong Zhi Xi
<desmondcheongzx@gmail.com> wrote:
>
> On 30/6/21 12:07 am, Daniel Vetter wrote:
> > On Tue, Jun 29, 2021 at 11:37:06AM +0800, Desmond Cheong Zhi Xi wrote:
> >> Currently, direct copies of drm_file->master pointers should be
> >> protected by drm_device.master_mutex when being dereferenced. This is
> >> because drm_file->master is not invariant for the lifetime of
> >> drm_file. If drm_file is not the creator of master, then
> >> drm_file->is_master is false, and a call to drm_setmaster_ioctl will
> >> invoke drm_new_set_master, which then allocates a new master for
> >> drm_file and puts the old master.
> >>
> >> Thus, without holding drm_device.master_mutex, the old value of
> >> drm_file->master could be freed while it is being used by another
> >> concurrent process.
> >>
> >> In drm_lease.c, there are multiple instances where drm_file->master is
> >> accessed and dereferenced while drm_device.master_mutex is not
> >> held. This makes drm_lease.c vulnerable to use-after-free bugs.
> >>
> >> We address this issue in 3 ways:
> >>
> >> 1. Clarify in the kerneldoc that drm_file->master is protected by
> >> drm_device.master_mutex.
> >>
> >> 2. Add a new drm_file_get_master() function that calls drm_master_get
> >> on drm_file->master while holding on to drm_device.master_mutex. Since
> >> drm_master_get increments the reference count of master, this
> >> prevents master from being freed until we unreference it with
> >> drm_master_put.
> >>
> >> 3. In each case where drm_file->master is directly accessed and
> >> eventually dereferenced in drm_lease.c, we wrap the access in a call
> >> to the new drm_file_get_master function, then unreference the master
> >> pointer once we are done using it.
> >>
> >> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> >
> > Series looks very nice, let's see what intel-gfx-ci says. You should get a
> > mail, but results are also here:
> >
> > https://patchwork.freedesktop.org/series/91969/#rev2
> >
> > One tiny comment below.
> >
> >> ---
> >>   drivers/gpu/drm/drm_auth.c  | 25 ++++++++++++
> >>   drivers/gpu/drm/drm_lease.c | 77 +++++++++++++++++++++++++++----------
> >>   include/drm/drm_auth.h      |  1 +
> >>   include/drm/drm_file.h      | 15 ++++++--
> >>   4 files changed, 95 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> >> index ab1863c5a5a0..c36a0b72be26 100644
> >> --- a/drivers/gpu/drm/drm_auth.c
> >> +++ b/drivers/gpu/drm/drm_auth.c
> >> @@ -384,6 +384,31 @@ struct drm_master *drm_master_get(struct drm_master *master)
> >>   }
> >>   EXPORT_SYMBOL(drm_master_get);
> >>
> >> +/**
> >> + * drm_file_get_master - reference &drm_file.master of @file_priv
> >> + * @file_priv: DRM file private
> >> + *
> >> + * Increments the reference count of @file_priv's &drm_file.master and returns
> >> + * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL.
> >> + *
> >> + * Master pointers returned from this function should be unreferenced using
> >> + * drm_master_put().
> >> + */
> >> +struct drm_master *drm_file_get_master(struct drm_file *file_priv)
> >> +{
> >> +    struct drm_master *master = NULL;
> >> +
> >> +    mutex_lock(&file_priv->minor->dev->master_mutex);
> >> +    if (!file_priv->master)
> >> +            goto unlock;
> >> +    master = drm_master_get(file_priv->master);
> >> +
> >> +unlock:
> >> +    mutex_unlock(&file_priv->minor->dev->master_mutex);
> >> +    return master;
> >> +}
> >> +EXPORT_SYMBOL(drm_file_get_master);
> >> +
> >>   static void drm_master_destroy(struct kref *kref)
> >>   {
> >>      struct drm_master *master = container_of(kref, struct drm_master, refcount);
> >> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> >> index 00fb433bcef1..cdcc87fa9685 100644
> >> --- a/drivers/gpu/drm/drm_lease.c
> >> +++ b/drivers/gpu/drm/drm_lease.c
> >> @@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> >>    */
> >>   bool _drm_lease_held(struct drm_file *file_priv, int id)
> >>   {
> >> -    if (!file_priv || !file_priv->master)
> >> +    bool ret;
> >> +    struct drm_master *master;
> >> +
> >> +    if (!file_priv)
> >>              return true;
> >>
> >> -    return _drm_lease_held_master(file_priv->master, id);
> >> +    master = drm_file_get_master(file_priv);
> >> +    if (master == NULL)
> >> +            return true;
> >> +    ret = _drm_lease_held_master(master, id);
> >> +    drm_master_put(&master);
> >> +
> >> +    return ret;
> >>   }
> >>
> >>   /**
> >> @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
> >>      struct drm_master *master;
> >>      bool ret;
> >>
> >> -    if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >> +    if (!file_priv)
> >>              return true;
> >>
> >> -    master = file_priv->master;
> >> +    master = drm_file_get_master(file_priv);
> >> +    if (master == NULL)
> >> +            return true;
> >> +    if (!master->lessor) {
> >> +            drm_master_put(&master);
> >> +            return true;
> >> +    }
> >>      mutex_lock(&master->dev->mode_config.idr_mutex);
> >>      ret = _drm_lease_held_master(master, id);
> >>      mutex_unlock(&master->dev->mode_config.idr_mutex);
> >> +    drm_master_put(&master);
> >>      return ret;
> >>   }
> >>
> >> @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> >>      int count_in, count_out;
> >>      uint32_t crtcs_out = 0;
> >>
> >> -    if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >> +    if (!file_priv)
> >>              return crtcs_in;
> >>
> >> -    master = file_priv->master;
> >> +    master = drm_file_get_master(file_priv);
> >> +    if (master == NULL)
> >> +            return crtcs_in;
> >> +    if (!master->lessor) {
> >> +            drm_master_put(&master);
> >> +            return crtcs_in;
> >> +    }
> >>      dev = master->dev;
> >>
> >>      count_in = count_out = 0;
> >> @@ -176,6 +198,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> >>              count_in++;
> >>      }
> >>      mutex_unlock(&master->dev->mode_config.idr_mutex);
> >> +    drm_master_put(&master);
> >>      return crtcs_out;
> >>   }
> >>
> >> @@ -489,7 +512,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> >>      size_t object_count;
> >>      int ret = 0;
> >>      struct idr leases;
> >> -    struct drm_master *lessor = lessor_priv->master;
> >> +    struct drm_master *lessor;
> >>      struct drm_master *lessee = NULL;
> >>      struct file *lessee_file = NULL;
> >>      struct file *lessor_file = lessor_priv->filp;
> >> @@ -501,12 +524,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> >>      if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>              return -EOPNOTSUPP;
> >>
> >> -    /* Do not allow sub-leases */
> >> -    if (lessor->lessor) {
> >> -            DRM_DEBUG_LEASE("recursive leasing not allowed\n");
> >> -            return -EINVAL;
> >> -    }
> >> -
> >>      /* need some objects */
> >>      if (cl->object_count == 0) {
> >>              DRM_DEBUG_LEASE("no objects in lease\n");
> >> @@ -518,12 +535,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> >>              return -EINVAL;
> >>      }
> >>
> >> +    lessor = drm_file_get_master(lessor_priv);
> >> +    /* Do not allow sub-leases */
> >> +    if (lessor->lessor) {
> >> +            DRM_DEBUG_LEASE("recursive leasing not allowed\n");
> >> +            ret = -EINVAL;
> >> +            goto out_lessor;
> >> +    }
> >> +
> >>      object_count = cl->object_count;
> >>
> >>      object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
> >>                      array_size(object_count, sizeof(__u32)));
> >> -    if (IS_ERR(object_ids))
> >> -            return PTR_ERR(object_ids);
> >> +    if (IS_ERR(object_ids)) {
> >> +            ret = PTR_ERR(object_ids);
> >> +            goto out_lessor;
> >> +    }
> >>
> >>      idr_init(&leases);
> >>
> >> @@ -534,14 +561,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> >>      if (ret) {
> >>              DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
> >>              idr_destroy(&leases);
> >> -            return ret;
> >> +            goto out_lessor;
> >>      }
> >>
> >>      /* Allocate a file descriptor for the lease */
> >>      fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
> >>      if (fd < 0) {
> >>              idr_destroy(&leases);
> >> -            return fd;
> >> +            ret = fd;
> >> +            goto out_lessor;
> >>      }
> >>
> >>      DRM_DEBUG_LEASE("Creating lease\n");
> >> @@ -577,6 +605,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> >>      /* Hook up the fd */
> >>      fd_install(fd, lessee_file);
> >>
> >> +    drm_master_put(&lessor);
> >>      DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
> >>      return 0;
> >>
> >> @@ -586,6 +615,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> >>   out_leases:
> >>      put_unused_fd(fd);
> >>
> >> +out_lessor:
> >> +    drm_master_put(&lessor);
> >>      DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
> >>      return ret;
> >>   }
> >> @@ -608,7 +639,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
> >>      struct drm_mode_list_lessees *arg = data;
> >>      __u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr);
> >>      __u32 count_lessees = arg->count_lessees;
> >> -    struct drm_master *lessor = lessor_priv->master, *lessee;
> >> +    struct drm_master *lessor, *lessee;
> >>      int count;
> >>      int ret = 0;
> >>
> >> @@ -619,6 +650,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
> >>      if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>              return -EOPNOTSUPP;
> >>
> >> +    lessor = drm_file_get_master(lessor_priv);
> >>      DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
> >>
> >>      mutex_lock(&dev->mode_config.idr_mutex);
> >> @@ -642,6 +674,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
> >>              arg->count_lessees = count;
> >>
> >>      mutex_unlock(&dev->mode_config.idr_mutex);
> >> +    drm_master_put(&lessor);
> >>
> >>      return ret;
> >>   }
> >> @@ -661,7 +694,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
> >>      struct drm_mode_get_lease *arg = data;
> >>      __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr);
> >>      __u32 count_objects = arg->count_objects;
> >> -    struct drm_master *lessee = lessee_priv->master;
> >> +    struct drm_master *lessee;
> >>      struct idr *object_idr;
> >>      int count;
> >>      void *entry;
> >> @@ -675,6 +708,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
> >>      if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>              return -EOPNOTSUPP;
> >>
> >> +    lessee = drm_file_get_master(lessee_priv);
> >>      DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
> >>
> >>      mutex_lock(&dev->mode_config.idr_mutex);
> >> @@ -702,6 +736,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
> >>              arg->count_objects = count;
> >>
> >>      mutex_unlock(&dev->mode_config.idr_mutex);
> >> +    drm_master_put(&lessee);
> >>
> >>      return ret;
> >>   }
> >> @@ -720,7 +755,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
> >>                              void *data, struct drm_file *lessor_priv)
> >>   {
> >>      struct drm_mode_revoke_lease *arg = data;
> >> -    struct drm_master *lessor = lessor_priv->master;
> >> +    struct drm_master *lessor;
> >>      struct drm_master *lessee;
> >>      int ret = 0;
> >>
> >> @@ -730,6 +765,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
> >>      if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>              return -EOPNOTSUPP;
> >>
> >> +    lessor = drm_file_get_master(lessor_priv);
> >>      mutex_lock(&dev->mode_config.idr_mutex);
> >>
> >>      lessee = _drm_find_lessee(lessor, arg->lessee_id);
> >> @@ -750,6 +786,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
> >>
> >>   fail:
> >>      mutex_unlock(&dev->mode_config.idr_mutex);
> >> +    drm_master_put(&lessor);
> >>
> >>      return ret;
> >>   }
> >> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> >> index 6bf8b2b78991..f99d3417f304 100644
> >> --- a/include/drm/drm_auth.h
> >> +++ b/include/drm/drm_auth.h
> >> @@ -107,6 +107,7 @@ struct drm_master {
> >>   };
> >>
> >>   struct drm_master *drm_master_get(struct drm_master *master);
> >> +struct drm_master *drm_file_get_master(struct drm_file *file_priv);
> >>   void drm_master_put(struct drm_master **master);
> >>   bool drm_is_current_master(struct drm_file *fpriv);
> >>
> >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> >> index b81b3bfb08c8..e9931fca4ab7 100644
> >> --- a/include/drm/drm_file.h
> >> +++ b/include/drm/drm_file.h
> >> @@ -226,9 +226,18 @@ struct drm_file {
> >>      /**
> >>       * @master:
> >>       *
> >> -     * Master this node is currently associated with. Only relevant if
> >> -     * drm_is_primary_client() returns true. Note that this only
> >> -     * matches &drm_device.master if the master is the currently active one.
> >> +     * Master this node is currently associated with. Protected by struct
> >> +     * &drm_device.master_mutex.
> >> +     *
> >> +     * Only relevant if drm_is_primary_client() returns true. Note that
> >> +     * this only matches &drm_device.master if the master is the currently
> >> +     * active one.
> >> +     *
> >> +     * When obtaining a copy of this pointer, it is recommended to either
> >
> > I found this a bit confusing, since I generally don't think of
> > dereferencing the pointer as "taking a copy". That's more for the entire
> > datastructure when you have a memcpy() call, or kmemdup() or something
> > like that. Also "it is recommended" is a bit weak if you get a
> > use-after-free if you dont :-)
> >
> > So instead "When dererencing this pointer either hold ... or use
> > drm_file_get_master() ...."
> >
> > Cheers, Daniel
> >
> >> +     * hold struct &drm_device.master_mutex for the duration of the
> >> +     * pointer's use, or to use drm_file_get_master() if struct
> >> +     * &drm_device.master_mutex is not currently held and there is no other
> >> +     * need to hold it. This prevents @master from being freed during use.
> >>       *
> >>       * See also @authentication and @is_master and the :ref:`section on
> >>       * primary nodes and authentication <drm_primary_node>`.
> >> --
> >> 2.25.1
> >>
> >
>
> Hi Daniel,
>
> Thanks for the suggestion, I'll clarify the kerneldoc accordingly.
>
> Regarding the results from intel-gfx-ci, it seems that the patch is
> inverting the lock hierarchy for
> &dev->master_mutex --> &dev->mode_config.idr_mutex
>
> Currently the dmesg warnings share a common call trace:
> drm_file_get_master+0x1b/0x70
> _drm_lease_held+0x21/0x70
> __drm_mode_object_find+0xd1/0xe0
>
> Looking at the functions that lock &dev->mode_config.idr_mutex, this
> should be the only instance of this lock order inversion.
>
> I'm thinking the call to _drm_lease_held can be moved outside of the
> &dev->mode_config.idr_mutex lock in __drm_mode_object. Any thoughts?

Uh very annoying. One of the callers of this is the atomic ioctl,
where we're calling this while holding drm_modeset_lock. The nesting
hierarchy is, from outermost lock to innermost: dev->master_mutex ->
dev->mode_config.mutex -> drm_modeset_lock. So I think we'll again
have an inversion, just moved it a bit :-(

But I'm also not 100% sure, so maybe type it up and see what happens?
Otherwise I think we need to figure out a solution for how we can
check leases without having to take the dev->master_mutex that
serializes a lot more things ... I think the fundamental problem we
have here is that dev->master_mutex serves 2 purposes: a) protecting
the pointers and just data consistency and b) synchronizing against
concurrent master changes where we think that's required. It's the
latter (through the fbdev emulation code) that causes all the
inversion problems, a) it's could easily nest very deeply in other
locks.

> diff --git a/drivers/gpu/drm/drm_mode_object.c
> b/drivers/gpu/drm/drm_mode_object.c
> index b26588b52795..63d35f1f98dd 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -146,16 +146,18 @@ struct drm_mode_object
> *__drm_mode_object_find(struct drm_device *dev,
>          if (obj && obj->id != id)
>                  obj = NULL;
>
> -       if (obj && drm_mode_object_lease_required(obj->type) &&
> -           !_drm_lease_held(file_priv, obj->id))
> -               obj = NULL;
> -
>          if (obj && obj->free_cb) {
>                  if (!kref_get_unless_zero(&obj->refcount))
>                          obj = NULL;
>          }
>          mutex_unlock(&dev->mode_config.idr_mutex);
>
> +       if (obj && drm_mode_object_lease_required(obj->type) &&
> +               !_drm_lease_held(file_priv, obj->id)) {
> +               drm_mode_object_put(obj);
> +               obj = NULL;
> +       }

Irrespective of all this I think this change here makes sense since in
untangels the master stuff from the lookup idr, and that's always
good. Maybe do this hunk as a separate patch, and I'll apply that as
prep work?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 3/3] drm: protect drm_master pointers in drm_lease.c

[Desmond Cheong Zhi Xi]

On 30/6/21 4:02 pm, Daniel Vetter wrote:
> On Wed, Jun 30, 2021 at 9:18 AM Desmond Cheong Zhi Xi
> <desmondcheongzx@gmail.com> wrote:
>>
>> On 30/6/21 12:07 am, Daniel Vetter wrote:
>>> On Tue, Jun 29, 2021 at 11:37:06AM +0800, Desmond Cheong Zhi Xi wrote:
>>>> Currently, direct copies of drm_file->master pointers should be
>>>> protected by drm_device.master_mutex when being dereferenced. This is
>>>> because drm_file->master is not invariant for the lifetime of
>>>> drm_file. If drm_file is not the creator of master, then
>>>> drm_file->is_master is false, and a call to drm_setmaster_ioctl will
>>>> invoke drm_new_set_master, which then allocates a new master for
>>>> drm_file and puts the old master.
>>>>
>>>> Thus, without holding drm_device.master_mutex, the old value of
>>>> drm_file->master could be freed while it is being used by another
>>>> concurrent process.
>>>>
>>>> In drm_lease.c, there are multiple instances where drm_file->master is
>>>> accessed and dereferenced while drm_device.master_mutex is not
>>>> held. This makes drm_lease.c vulnerable to use-after-free bugs.
>>>>
>>>> We address this issue in 3 ways:
>>>>
>>>> 1. Clarify in the kerneldoc that drm_file->master is protected by
>>>> drm_device.master_mutex.
>>>>
>>>> 2. Add a new drm_file_get_master() function that calls drm_master_get
>>>> on drm_file->master while holding on to drm_device.master_mutex. Since
>>>> drm_master_get increments the reference count of master, this
>>>> prevents master from being freed until we unreference it with
>>>> drm_master_put.
>>>>
>>>> 3. In each case where drm_file->master is directly accessed and
>>>> eventually dereferenced in drm_lease.c, we wrap the access in a call
>>>> to the new drm_file_get_master function, then unreference the master
>>>> pointer once we are done using it.
>>>>
>>>> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>>>
>>> Series looks very nice, let's see what intel-gfx-ci says. You should get a
>>> mail, but results are also here:
>>>
>>> https://patchwork.freedesktop.org/series/91969/#rev2
>>>
>>> One tiny comment below.
>>>
>>>> ---
>>>>    drivers/gpu/drm/drm_auth.c  | 25 ++++++++++++
>>>>    drivers/gpu/drm/drm_lease.c | 77 +++++++++++++++++++++++++++----------
>>>>    include/drm/drm_auth.h      |  1 +
>>>>    include/drm/drm_file.h      | 15 ++++++--
>>>>    4 files changed, 95 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>>>> index ab1863c5a5a0..c36a0b72be26 100644
>>>> --- a/drivers/gpu/drm/drm_auth.c
>>>> +++ b/drivers/gpu/drm/drm_auth.c
>>>> @@ -384,6 +384,31 @@ struct drm_master *drm_master_get(struct drm_master *master)
>>>>    }
>>>>    EXPORT_SYMBOL(drm_master_get);
>>>>
>>>> +/**
>>>> + * drm_file_get_master - reference &drm_file.master of @file_priv
>>>> + * @file_priv: DRM file private
>>>> + *
>>>> + * Increments the reference count of @file_priv's &drm_file.master and returns
>>>> + * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL.
>>>> + *
>>>> + * Master pointers returned from this function should be unreferenced using
>>>> + * drm_master_put().
>>>> + */
>>>> +struct drm_master *drm_file_get_master(struct drm_file *file_priv)
>>>> +{
>>>> +    struct drm_master *master = NULL;
>>>> +
>>>> +    mutex_lock(&file_priv->minor->dev->master_mutex);
>>>> +    if (!file_priv->master)
>>>> +            goto unlock;
>>>> +    master = drm_master_get(file_priv->master);
>>>> +
>>>> +unlock:
>>>> +    mutex_unlock(&file_priv->minor->dev->master_mutex);
>>>> +    return master;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_file_get_master);
>>>> +
>>>>    static void drm_master_destroy(struct kref *kref)
>>>>    {
>>>>       struct drm_master *master = container_of(kref, struct drm_master, refcount);
>>>> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
>>>> index 00fb433bcef1..cdcc87fa9685 100644
>>>> --- a/drivers/gpu/drm/drm_lease.c
>>>> +++ b/drivers/gpu/drm/drm_lease.c
>>>> @@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id)
>>>>     */
>>>>    bool _drm_lease_held(struct drm_file *file_priv, int id)
>>>>    {
>>>> -    if (!file_priv || !file_priv->master)
>>>> +    bool ret;
>>>> +    struct drm_master *master;
>>>> +
>>>> +    if (!file_priv)
>>>>               return true;
>>>>
>>>> -    return _drm_lease_held_master(file_priv->master, id);
>>>> +    master = drm_file_get_master(file_priv);
>>>> +    if (master == NULL)
>>>> +            return true;
>>>> +    ret = _drm_lease_held_master(master, id);
>>>> +    drm_master_put(&master);
>>>> +
>>>> +    return ret;
>>>>    }
>>>>
>>>>    /**
>>>> @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>>>>       struct drm_master *master;
>>>>       bool ret;
>>>>
>>>> -    if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>>>> +    if (!file_priv)
>>>>               return true;
>>>>
>>>> -    master = file_priv->master;
>>>> +    master = drm_file_get_master(file_priv);
>>>> +    if (master == NULL)
>>>> +            return true;
>>>> +    if (!master->lessor) {
>>>> +            drm_master_put(&master);
>>>> +            return true;
>>>> +    }
>>>>       mutex_lock(&master->dev->mode_config.idr_mutex);
>>>>       ret = _drm_lease_held_master(master, id);
>>>>       mutex_unlock(&master->dev->mode_config.idr_mutex);
>>>> +    drm_master_put(&master);
>>>>       return ret;
>>>>    }
>>>>
>>>> @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>>>>       int count_in, count_out;
>>>>       uint32_t crtcs_out = 0;
>>>>
>>>> -    if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>>>> +    if (!file_priv)
>>>>               return crtcs_in;
>>>>
>>>> -    master = file_priv->master;
>>>> +    master = drm_file_get_master(file_priv);
>>>> +    if (master == NULL)
>>>> +            return crtcs_in;
>>>> +    if (!master->lessor) {
>>>> +            drm_master_put(&master);
>>>> +            return crtcs_in;
>>>> +    }
>>>>       dev = master->dev;
>>>>
>>>>       count_in = count_out = 0;
>>>> @@ -176,6 +198,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>>>>               count_in++;
>>>>       }
>>>>       mutex_unlock(&master->dev->mode_config.idr_mutex);
>>>> +    drm_master_put(&master);
>>>>       return crtcs_out;
>>>>    }
>>>>
>>>> @@ -489,7 +512,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>>>       size_t object_count;
>>>>       int ret = 0;
>>>>       struct idr leases;
>>>> -    struct drm_master *lessor = lessor_priv->master;
>>>> +    struct drm_master *lessor;
>>>>       struct drm_master *lessee = NULL;
>>>>       struct file *lessee_file = NULL;
>>>>       struct file *lessor_file = lessor_priv->filp;
>>>> @@ -501,12 +524,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>>>       if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>               return -EOPNOTSUPP;
>>>>
>>>> -    /* Do not allow sub-leases */
>>>> -    if (lessor->lessor) {
>>>> -            DRM_DEBUG_LEASE("recursive leasing not allowed\n");
>>>> -            return -EINVAL;
>>>> -    }
>>>> -
>>>>       /* need some objects */
>>>>       if (cl->object_count == 0) {
>>>>               DRM_DEBUG_LEASE("no objects in lease\n");
>>>> @@ -518,12 +535,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>>>               return -EINVAL;
>>>>       }
>>>>
>>>> +    lessor = drm_file_get_master(lessor_priv);
>>>> +    /* Do not allow sub-leases */
>>>> +    if (lessor->lessor) {
>>>> +            DRM_DEBUG_LEASE("recursive leasing not allowed\n");
>>>> +            ret = -EINVAL;
>>>> +            goto out_lessor;
>>>> +    }
>>>> +
>>>>       object_count = cl->object_count;
>>>>
>>>>       object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
>>>>                       array_size(object_count, sizeof(__u32)));
>>>> -    if (IS_ERR(object_ids))
>>>> -            return PTR_ERR(object_ids);
>>>> +    if (IS_ERR(object_ids)) {
>>>> +            ret = PTR_ERR(object_ids);
>>>> +            goto out_lessor;
>>>> +    }
>>>>
>>>>       idr_init(&leases);
>>>>
>>>> @@ -534,14 +561,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>>>       if (ret) {
>>>>               DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
>>>>               idr_destroy(&leases);
>>>> -            return ret;
>>>> +            goto out_lessor;
>>>>       }
>>>>
>>>>       /* Allocate a file descriptor for the lease */
>>>>       fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
>>>>       if (fd < 0) {
>>>>               idr_destroy(&leases);
>>>> -            return fd;
>>>> +            ret = fd;
>>>> +            goto out_lessor;
>>>>       }
>>>>
>>>>       DRM_DEBUG_LEASE("Creating lease\n");
>>>> @@ -577,6 +605,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>>>       /* Hook up the fd */
>>>>       fd_install(fd, lessee_file);
>>>>
>>>> +    drm_master_put(&lessor);
>>>>       DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>>>>       return 0;
>>>>
>>>> @@ -586,6 +615,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>>>    out_leases:
>>>>       put_unused_fd(fd);
>>>>
>>>> +out_lessor:
>>>> +    drm_master_put(&lessor);
>>>>       DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
>>>>       return ret;
>>>>    }
>>>> @@ -608,7 +639,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>>>       struct drm_mode_list_lessees *arg = data;
>>>>       __u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr);
>>>>       __u32 count_lessees = arg->count_lessees;
>>>> -    struct drm_master *lessor = lessor_priv->master, *lessee;
>>>> +    struct drm_master *lessor, *lessee;
>>>>       int count;
>>>>       int ret = 0;
>>>>
>>>> @@ -619,6 +650,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>>>       if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>               return -EOPNOTSUPP;
>>>>
>>>> +    lessor = drm_file_get_master(lessor_priv);
>>>>       DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
>>>>
>>>>       mutex_lock(&dev->mode_config.idr_mutex);
>>>> @@ -642,6 +674,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>>>               arg->count_lessees = count;
>>>>
>>>>       mutex_unlock(&dev->mode_config.idr_mutex);
>>>> +    drm_master_put(&lessor);
>>>>
>>>>       return ret;
>>>>    }
>>>> @@ -661,7 +694,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>>>       struct drm_mode_get_lease *arg = data;
>>>>       __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr);
>>>>       __u32 count_objects = arg->count_objects;
>>>> -    struct drm_master *lessee = lessee_priv->master;
>>>> +    struct drm_master *lessee;
>>>>       struct idr *object_idr;
>>>>       int count;
>>>>       void *entry;
>>>> @@ -675,6 +708,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>>>       if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>               return -EOPNOTSUPP;
>>>>
>>>> +    lessee = drm_file_get_master(lessee_priv);
>>>>       DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
>>>>
>>>>       mutex_lock(&dev->mode_config.idr_mutex);
>>>> @@ -702,6 +736,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>>>               arg->count_objects = count;
>>>>
>>>>       mutex_unlock(&dev->mode_config.idr_mutex);
>>>> +    drm_master_put(&lessee);
>>>>
>>>>       return ret;
>>>>    }
>>>> @@ -720,7 +755,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>>>                               void *data, struct drm_file *lessor_priv)
>>>>    {
>>>>       struct drm_mode_revoke_lease *arg = data;
>>>> -    struct drm_master *lessor = lessor_priv->master;
>>>> +    struct drm_master *lessor;
>>>>       struct drm_master *lessee;
>>>>       int ret = 0;
>>>>
>>>> @@ -730,6 +765,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>>>       if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>               return -EOPNOTSUPP;
>>>>
>>>> +    lessor = drm_file_get_master(lessor_priv);
>>>>       mutex_lock(&dev->mode_config.idr_mutex);
>>>>
>>>>       lessee = _drm_find_lessee(lessor, arg->lessee_id);
>>>> @@ -750,6 +786,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>>>
>>>>    fail:
>>>>       mutex_unlock(&dev->mode_config.idr_mutex);
>>>> +    drm_master_put(&lessor);
>>>>
>>>>       return ret;
>>>>    }
>>>> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
>>>> index 6bf8b2b78991..f99d3417f304 100644
>>>> --- a/include/drm/drm_auth.h
>>>> +++ b/include/drm/drm_auth.h
>>>> @@ -107,6 +107,7 @@ struct drm_master {
>>>>    };
>>>>
>>>>    struct drm_master *drm_master_get(struct drm_master *master);
>>>> +struct drm_master *drm_file_get_master(struct drm_file *file_priv);
>>>>    void drm_master_put(struct drm_master **master);
>>>>    bool drm_is_current_master(struct drm_file *fpriv);
>>>>
>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>> index b81b3bfb08c8..e9931fca4ab7 100644
>>>> --- a/include/drm/drm_file.h
>>>> +++ b/include/drm/drm_file.h
>>>> @@ -226,9 +226,18 @@ struct drm_file {
>>>>       /**
>>>>        * @master:
>>>>        *
>>>> -     * Master this node is currently associated with. Only relevant if
>>>> -     * drm_is_primary_client() returns true. Note that this only
>>>> -     * matches &drm_device.master if the master is the currently active one.
>>>> +     * Master this node is currently associated with. Protected by struct
>>>> +     * &drm_device.master_mutex.
>>>> +     *
>>>> +     * Only relevant if drm_is_primary_client() returns true. Note that
>>>> +     * this only matches &drm_device.master if the master is the currently
>>>> +     * active one.
>>>> +     *
>>>> +     * When obtaining a copy of this pointer, it is recommended to either
>>>
>>> I found this a bit confusing, since I generally don't think of
>>> dereferencing the pointer as "taking a copy". That's more for the entire
>>> datastructure when you have a memcpy() call, or kmemdup() or something
>>> like that. Also "it is recommended" is a bit weak if you get a
>>> use-after-free if you dont :-)
>>>
>>> So instead "When dererencing this pointer either hold ... or use
>>> drm_file_get_master() ...."
>>>
>>> Cheers, Daniel
>>>
>>>> +     * hold struct &drm_device.master_mutex for the duration of the
>>>> +     * pointer's use, or to use drm_file_get_master() if struct
>>>> +     * &drm_device.master_mutex is not currently held and there is no other
>>>> +     * need to hold it. This prevents @master from being freed during use.
>>>>        *
>>>>        * See also @authentication and @is_master and the :ref:`section on
>>>>        * primary nodes and authentication <drm_primary_node>`.
>>>> --
>>>> 2.25.1
>>>>
>>>
>>
>> Hi Daniel,
>>
>> Thanks for the suggestion, I'll clarify the kerneldoc accordingly.
>>
>> Regarding the results from intel-gfx-ci, it seems that the patch is
>> inverting the lock hierarchy for
>> &dev->master_mutex --> &dev->mode_config.idr_mutex
>>
>> Currently the dmesg warnings share a common call trace:
>> drm_file_get_master+0x1b/0x70
>> _drm_lease_held+0x21/0x70
>> __drm_mode_object_find+0xd1/0xe0
>>
>> Looking at the functions that lock &dev->mode_config.idr_mutex, this
>> should be the only instance of this lock order inversion.
>>
>> I'm thinking the call to _drm_lease_held can be moved outside of the
>> &dev->mode_config.idr_mutex lock in __drm_mode_object. Any thoughts?
> 
> Uh very annoying. One of the callers of this is the atomic ioctl,
> where we're calling this while holding drm_modeset_lock. The nesting
> hierarchy is, from outermost lock to innermost: dev->master_mutex ->
> dev->mode_config.mutex -> drm_modeset_lock. So I think we'll again
> have an inversion, just moved it a bit :-(
> 
> But I'm also not 100% sure, so maybe type it up and see what happens?
> Otherwise I think we need to figure out a solution for how we can
> check leases without having to take the dev->master_mutex that
> serializes a lot more things ... I think the fundamental problem we
> have here is that dev->master_mutex serves 2 purposes: a) protecting
> the pointers and just data consistency and b) synchronizing against
> concurrent master changes where we think that's required. It's the
> latter (through the fbdev emulation code) that causes all the
> inversion problems, a) it's could easily nest very deeply in other
> locks.
> 

Ah I see, that sounds thorny. Just from inspecting the code I can't find 
the inversion, so I'll write up the v6 series for testing first.

>> diff --git a/drivers/gpu/drm/drm_mode_object.c
>> b/drivers/gpu/drm/drm_mode_object.c
>> index b26588b52795..63d35f1f98dd 100644
>> --- a/drivers/gpu/drm/drm_mode_object.c
>> +++ b/drivers/gpu/drm/drm_mode_object.c
>> @@ -146,16 +146,18 @@ struct drm_mode_object
>> *__drm_mode_object_find(struct drm_device *dev,
>>           if (obj && obj->id != id)
>>                   obj = NULL;
>>
>> -       if (obj && drm_mode_object_lease_required(obj->type) &&
>> -           !_drm_lease_held(file_priv, obj->id))
>> -               obj = NULL;
>> -
>>           if (obj && obj->free_cb) {
>>                   if (!kref_get_unless_zero(&obj->refcount))
>>                           obj = NULL;
>>           }
>>           mutex_unlock(&dev->mode_config.idr_mutex);
>>
>> +       if (obj && drm_mode_object_lease_required(obj->type) &&
>> +               !_drm_lease_held(file_priv, obj->id)) {
>> +               drm_mode_object_put(obj);
>> +               obj = NULL;
>> +       }
> 
> Irrespective of all this I think this change here makes sense since in
> untangels the master stuff from the lookup idr, and that's always
> good. Maybe do this hunk as a separate patch, and I'll apply that as
> prep work?
> -Daniel
> 

Sounds good to me, I'll add the new patch to the series.

Re: [PATCH v5 3/3] drm: protect drm_master pointers in drm_lease.c

[Emil Velikov]

Hi Desmond,

Couple of small suggestions, with those the series is:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

On Tue, 29 Jun 2021 at 04:38, Desmond Cheong Zhi Xi
<desmondcheongzx@gmail.com> wrote:

> @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>         struct drm_master *master;
>         bool ret;
>
> -       if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> +       if (!file_priv)
>                 return true;
>
> -       master = file_priv->master;
> +       master = drm_file_get_master(file_priv);
> +       if (master == NULL)
> +               return true;
> +       if (!master->lessor) {
> +               drm_master_put(&master);
> +               return true;

Let's add a "ret = true; goto unlock;" here, so we can have a single
drm_master_put() in the function.
Nearly all code paths touched by this patch already follow this approach.

> @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>         int count_in, count_out;
>         uint32_t crtcs_out = 0;
>
> -       if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> +       if (!file_priv)
>                 return crtcs_in;
>
> -       master = file_priv->master;
> +       master = drm_file_get_master(file_priv);
> +       if (master == NULL)
> +               return crtcs_in;
> +       if (!master->lessor) {
> +               drm_master_put(&master);
> +               return crtcs_in;

Ditto

Thanks
Emil

Re: [PATCH v5 3/3] drm: protect drm_master pointers in drm_lease.c

[Desmond Cheong Zhi Xi]

On 30/6/21 8:16 am, Emil Velikov wrote:
> Hi Desmond,
> 
> Couple of small suggestions, with those the series is:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> 
> On Tue, 29 Jun 2021 at 04:38, Desmond Cheong Zhi Xi
> <desmondcheongzx@gmail.com> wrote:
> 
>> @@ -128,13 +137,20 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>>          struct drm_master *master;
>>          bool ret;
>>
>> -       if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>> +       if (!file_priv)
>>                  return true;
>>
>> -       master = file_priv->master;
>> +       master = drm_file_get_master(file_priv);
>> +       if (master == NULL)
>> +               return true;
>> +       if (!master->lessor) {
>> +               drm_master_put(&master);
>> +               return true;
> 
> Let's add a "ret = true; goto unlock;" here, so we can have a single
> drm_master_put() in the function.
> Nearly all code paths touched by this patch already follow this approach.
> 
>> @@ -154,10 +170,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>>          int count_in, count_out;
>>          uint32_t crtcs_out = 0;
>>
>> -       if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>> +       if (!file_priv)
>>                  return crtcs_in;
>>
>> -       master = file_priv->master;
>> +       master = drm_file_get_master(file_priv);
>> +       if (master == NULL)
>> +               return crtcs_in;
>> +       if (!master->lessor) {
>> +               drm_master_put(&master);
>> +               return crtcs_in;
> 
> Ditto
> 
> Thanks
> Emil
> 

Sounds good to me, I'll revise these functions. Thanks for the review 
and suggestions, Emil.

Best wishes,
Desmond