Hi,
In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this.
Overall, this 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 five 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. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info().
3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes.
5. 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.
v7 -> v8:
- Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI.
- Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI.
v6 -> v7:
- Modify code alignment as suggested by the intel-gfx CI.
- Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI.
- Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI.
v5 -> v6:
- Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find.
- Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter.
- Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov.
- Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI.
v4 -> v5:
- Add a new patch to the series that 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.
v3 -> v4:
- 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/
- 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.
- Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter.
- 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.
v2 -> v3:
- 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.
- 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.
v1 -> v2:
- 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 (5):
drm: avoid circular locks in drm_mode_getconnector
drm: avoid blocking in drm_clients_info's rcu section
drm: add a locked version of drm_is_current_master
drm: serialize drm_file.master with a new spinlock
drm: protect drm_master pointers in drm_lease.c
drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++---------
drivers/gpu/drm/drm_connector.c | 5 +-
drivers/gpu/drm/drm_debugfs.c | 3 +-
drivers/gpu/drm/drm_file.c | 1 +
drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++-------
include/drm/drm_auth.h | 1 +
include/drm/drm_file.h | 18 +++++--
7 files changed, 152 insertions(+), 50 deletions(-)
--
2.25.1
Inside drm_clients_info, the rcu_read_lock is held to lock
pid_task()->comm. However, within this protected section, a call to
drm_is_current_master is made, which involves a mutex lock in a future
patch. However, this is illegal because the mutex lock might block
while in the RCU read-side critical section.
Since drm_is_current_master isn't protected by rcu_read_lock, we avoid
this by moving it out of the RCU critical section.
The following report came from intel-gfx ci's
igt@debugfs_test@read_all_entries testcase:
=============================
[ BUG: Invalid wait context ]
5.13.0-CI-Patchwork_20515+ #1 Tainted: G W
-----------------------------
debugfs_test/1101 is trying to lock:
ffff888132d901a8 (&dev->master_mutex){+.+.}-{3:3}, at:
drm_is_current_master+0x1e/0x50
other info that might help us debug this:
context-{4:4}
3 locks held by debugfs_test/1101:
#0: ffff88810fdffc90 (&p->lock){+.+.}-{3:3}, at:
seq_read_iter+0x53/0x3b0
#1: ffff888132d90240 (&dev->filelist_mutex){+.+.}-{3:3}, at:
drm_clients_info+0x63/0x2a0
#2: ffffffff82734220 (rcu_read_lock){....}-{1:2}, at:
drm_clients_info+0x1b1/0x2a0
stack backtrace:
CPU: 8 PID: 1101 Comm: debugfs_test Tainted: G W
5.13.0-CI-Patchwork_20515+ #1
Hardware name: Intel Corporation CometLake Client Platform/CometLake S
UDIMM (ERB/CRB), BIOS CMLSFWR1.R00.1263.D00.1906260926 06/26/2019
Call Trace:
dump_stack+0x7f/0xad
__lock_acquire.cold.78+0x2af/0x2ca
lock_acquire+0xd3/0x300
? drm_is_current_master+0x1e/0x50
? __mutex_lock+0x76/0x970
? lockdep_hardirqs_on+0xbf/0x130
__mutex_lock+0xab/0x970
? drm_is_current_master+0x1e/0x50
? drm_is_current_master+0x1e/0x50
? drm_is_current_master+0x1e/0x50
drm_is_current_master+0x1e/0x50
drm_clients_info+0x107/0x2a0
seq_read_iter+0x178/0x3b0
seq_read+0x104/0x150
full_proxy_read+0x4e/0x80
vfs_read+0xa5/0x1b0
ksys_read+0x5a/0xd0
do_syscall_64+0x39/0xb0
entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
drivers/gpu/drm/drm_debugfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 3d7182001004..b0a826489488 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -91,6 +91,7 @@ static int drm_clients_info(struct seq_file *m, void *data)
mutex_lock(&dev->filelist_mutex);
list_for_each_entry_reverse(priv, &dev->filelist, lhead) {
struct task_struct *task;
+ bool is_current_master = drm_is_current_master(priv);
rcu_read_lock(); /* locks pid_task()->comm */
task = pid_task(priv->pid, PIDTYPE_PID);
@@ -99,7 +100,7 @@ static int drm_clients_info(struct seq_file *m, void *data)
task ? task->comm : "<unknown>",
pid_vnr(priv->pid),
priv->minor->index,
- drm_is_current_master(priv) ? 'y' : 'n',
+ is_current_master ? 'y' : 'n',
priv->authenticated ? 'y' : 'n',
from_kuid_munged(seq_user_ns(m), uid),
priv->magic);
--
2.25.1
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 <[email protected]>
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
---
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
Currently, 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.
However, it is not always possible to lock drm_device.master_mutex to
dereference drm_file.master. Through the fbdev emulation code, this
might occur in a deep nest of other locks. But drm_device.master_mutex
is also the outermost lock in the nesting hierarchy, so this leads to
potential deadlocks.
To address this, we introduce a new spin lock at the bottom of the
lock hierarchy that only serializes drm_file.master. With this change,
the value of drm_file.master changes only when both
drm_device.master_mutex and drm_file.master_lookup_lock are
held. Hence, any process holding either of those locks can ensure that
the value of drm_file.master will not change concurrently.
Since no lock depends on the new drm_file.master_lookup_lock, when
drm_file.master is dereferenced, but drm_device.master_mutex cannot be
held, we can safely protect the master pointer with
drm_file.master_lookup_lock.
Reported-by: Daniel Vetter <[email protected]>
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
drivers/gpu/drm/drm_auth.c | 17 +++++++++++------
drivers/gpu/drm/drm_file.c | 1 +
include/drm/drm_file.h | 12 +++++++++---
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index ab1863c5a5a0..30a239901b36 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -164,16 +164,18 @@ static void drm_set_master(struct drm_device *dev, struct drm_file *fpriv,
static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
{
struct drm_master *old_master;
+ struct drm_master *new_master;
lockdep_assert_held_once(&dev->master_mutex);
WARN_ON(fpriv->is_master);
old_master = fpriv->master;
- fpriv->master = drm_master_create(dev);
- if (!fpriv->master) {
- fpriv->master = old_master;
+ new_master = drm_master_create(dev);
+ if (!new_master)
return -ENOMEM;
- }
+ spin_lock(&fpriv->master_lookup_lock);
+ fpriv->master = new_master;
+ spin_unlock(&fpriv->master_lookup_lock);
fpriv->is_master = 1;
fpriv->authenticated = 1;
@@ -332,10 +334,13 @@ int drm_master_open(struct drm_file *file_priv)
* any master object for render clients
*/
mutex_lock(&dev->master_mutex);
- if (!dev->master)
+ if (!dev->master) {
ret = drm_new_set_master(dev, file_priv);
- else
+ } else {
+ spin_lock(&file_priv->master_lookup_lock);
file_priv->master = drm_master_get(dev->master);
+ spin_unlock(&file_priv->master_lookup_lock);
+ }
mutex_unlock(&dev->master_mutex);
return ret;
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index d4f0bac6f8f8..ceb1a9723855 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -176,6 +176,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
init_waitqueue_head(&file->event_wait);
file->event_space = 4096; /* set aside 4k for event buffer */
+ spin_lock_init(&file->master_lookup_lock);
mutex_init(&file->event_read_lock);
if (drm_core_check_feature(dev, DRIVER_GEM))
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index b81b3bfb08c8..9b82988e3427 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -226,15 +226,21 @@ 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, and serialized by @master_lookup_lock.
+ *
+ * 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.
*
* See also @authentication and @is_master and the :ref:`section on
* primary nodes and authentication <drm_primary_node>`.
*/
struct drm_master *master;
+ /** @master_lock: Serializes @master. */
+ spinlock_t master_lookup_lock;
+
/** @pid: Process that opened this file. */
struct pid *pid;
--
2.25.1
drm_file->master pointers should be protected by
drm_device.master_mutex or drm_file.master_lookup_lock when being
dereferenced.
However, in drm_lease.c, there are multiple instances where
drm_file->master is accessed and dereferenced while neither lock is
held. This makes drm_lease.c vulnerable to use-after-free bugs.
We address this issue in 2 ways:
1. Add a new drm_file_get_master() function that calls drm_master_get
on drm_file->master while holding on to
drm_file.master_lookup_lock. Since drm_master_get increments the
reference count of master, this prevents master from being freed until
we unreference it with drm_master_put.
2. 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 <[email protected]>
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
---
drivers/gpu/drm/drm_auth.c | 25 ++++++++++++
drivers/gpu/drm/drm_lease.c | 81 ++++++++++++++++++++++++++++---------
include/drm/drm_auth.h | 1 +
include/drm/drm_file.h | 6 +++
4 files changed, 93 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 30a239901b36..f00354bec3fb 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -389,6 +389,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;
+
+ spin_lock(&file_priv->master_lookup_lock);
+ if (!file_priv->master)
+ goto unlock;
+ master = drm_master_get(file_priv->master);
+
+unlock:
+ spin_unlock(&file_priv->master_lookup_lock);
+ 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..92eac73d9001 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)
+ return true;
+ ret = _drm_lease_held_master(master, id);
+ drm_master_put(&master);
+
+ return ret;
}
/**
@@ -128,13 +137,22 @@ 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)
+ return true;
+ if (!master->lessor) {
+ ret = true;
+ goto out;
+ }
mutex_lock(&master->dev->mode_config.idr_mutex);
ret = _drm_lease_held_master(master, id);
mutex_unlock(&master->dev->mode_config.idr_mutex);
+
+out:
+ drm_master_put(&master);
return ret;
}
@@ -154,10 +172,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)
+ return crtcs_in;
+ if (!master->lessor) {
+ crtcs_out = crtcs_in;
+ goto out;
+ }
dev = master->dev;
count_in = count_out = 0;
@@ -176,6 +200,9 @@ 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);
+
+out:
+ drm_master_put(&master);
return crtcs_out;
}
@@ -489,7 +516,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 +528,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 +539,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 +565,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 +609,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 +619,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 +643,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 +654,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 +678,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 +698,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 +712,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 +740,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 +759,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 +769,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 +790,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 9b82988e3427..726cfe0ff5f5 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -233,6 +233,12 @@ struct drm_file {
* this only matches &drm_device.master if the master is the currently
* active one.
*
+ * When dereferencing this pointer, either hold struct
+ * &drm_device.master_mutex for the duration of the pointer's use, or
+ * 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
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 <[email protected]>
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
---
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
On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote:
> Hi,
>
> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this.
>
> Overall, this 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 five 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. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info().
>
> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
>
> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes.
>
> 5. 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.
>
> v7 -> v8:
> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI.
> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI.
>
> v6 -> v7:
> - Modify code alignment as suggested by the intel-gfx CI.
> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI.
> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI.
>
> v5 -> v6:
> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find.
> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter.
> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov.
> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI.
>
> v4 -> v5:
> - Add a new patch to the series that 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.
>
> v3 -> v4:
> - 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/
> - 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.
> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter.
> - 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.
>
> v2 -> v3:
> - 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.
> - 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.
>
> v1 -> v2:
> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov.
Apologies for the delay, I missed your series. Maybe just ping next time
around there's silence.
Looks all great, merged to drm-misc-next. Given how complex this was I'm
vary of just pushing this to -fixes without some solid testing.
One thing I noticed is that drm_is_current_master could just use the
spinlock, since it's only doing a read access. Care to type up that patch?
Also, do you plan to look into that idea we've discussed to flush pending
access when we revoke a master or a lease? I think that would be really
nice improvement here.
-Daniel
>
> Desmond Cheong Zhi Xi (5):
> drm: avoid circular locks in drm_mode_getconnector
> drm: avoid blocking in drm_clients_info's rcu section
> drm: add a locked version of drm_is_current_master
> drm: serialize drm_file.master with a new spinlock
> drm: protect drm_master pointers in drm_lease.c
>
> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++---------
> drivers/gpu/drm/drm_connector.c | 5 +-
> drivers/gpu/drm/drm_debugfs.c | 3 +-
> drivers/gpu/drm/drm_file.c | 1 +
> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++-------
> include/drm/drm_auth.h | 1 +
> include/drm/drm_file.h | 18 +++++--
> 7 files changed, 152 insertions(+), 50 deletions(-)
>
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 21/7/21 2:24 am, Daniel Vetter wrote:
> On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote:
>> Hi,
>>
>> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this.
>>
>> Overall, this 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 five 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. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info().
>>
>> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
>>
>> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes.
>>
>> 5. 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.
>>
>> v7 -> v8:
>> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI.
>> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI.
>>
>> v6 -> v7:
>> - Modify code alignment as suggested by the intel-gfx CI.
>> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI.
>> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI.
>>
>> v5 -> v6:
>> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find.
>> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter.
>> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov.
>> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI.
>>
>> v4 -> v5:
>> - Add a new patch to the series that 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.
>>
>> v3 -> v4:
>> - 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/
>> - 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.
>> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter.
>> - 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.
>>
>> v2 -> v3:
>> - 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.
>> - 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.
>>
>> v1 -> v2:
>> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov.
>
> Apologies for the delay, I missed your series. Maybe just ping next time
> around there's silence.
>
> Looks all great, merged to drm-misc-next. Given how complex this was I'm
> vary of just pushing this to -fixes without some solid testing.
>
Hi Daniel,
Thanks for merging, more testing definitely sounds good to me.
> One thing I noticed is that drm_is_current_master could just use the
> spinlock, since it's only doing a read access. Care to type up that patch?
>
I thought about this too, but I'm not sure if that's the best solution.
drm_is_current_master calls drm_lease_owner which then walks up the tree
of master lessors. The spinlock protects the master of the current drm
file, but subsequent lessors aren't protected without holding the
device's master mutex.
> Also, do you plan to look into that idea we've discussed to flush pending
> access when we revoke a master or a lease? I think that would be really
> nice improvement here.
> -Daniel
>
Yup, now that the potential UAFs are addressed (hopefully), I'll take a
closer look and propose a patch for this.
Best wishes,
Desmond
>>
>> Desmond Cheong Zhi Xi (5):
>> drm: avoid circular locks in drm_mode_getconnector
>> drm: avoid blocking in drm_clients_info's rcu section
>> drm: add a locked version of drm_is_current_master
>> drm: serialize drm_file.master with a new spinlock
>> drm: protect drm_master pointers in drm_lease.c
>>
>> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++---------
>> drivers/gpu/drm/drm_connector.c | 5 +-
>> drivers/gpu/drm/drm_debugfs.c | 3 +-
>> drivers/gpu/drm/drm_file.c | 1 +
>> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++-------
>> include/drm/drm_auth.h | 1 +
>> include/drm/drm_file.h | 18 +++++--
>> 7 files changed, 152 insertions(+), 50 deletions(-)
>>
>> --
>> 2.25.1
>>
>
On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi
<[email protected]> wrote:
> On 21/7/21 2:24 am, Daniel Vetter wrote:
> > On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote:
> >> Hi,
> >>
> >> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this.
> >>
> >> Overall, this 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 five 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. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info().
> >>
> >> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
> >>
> >> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes.
> >>
> >> 5. 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.
> >>
> >> v7 -> v8:
> >> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI.
> >> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI.
> >>
> >> v6 -> v7:
> >> - Modify code alignment as suggested by the intel-gfx CI.
> >> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI.
> >> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI.
> >>
> >> v5 -> v6:
> >> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find.
> >> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter.
> >> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov.
> >> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI.
> >>
> >> v4 -> v5:
> >> - Add a new patch to the series that 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.
> >>
> >> v3 -> v4:
> >> - 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/
> >> - 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.
> >> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter.
> >> - 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.
> >>
> >> v2 -> v3:
> >> - 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.
> >> - 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.
> >>
> >> v1 -> v2:
> >> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov.
> >
> > Apologies for the delay, I missed your series. Maybe just ping next time
> > around there's silence.
> >
> > Looks all great, merged to drm-misc-next. Given how complex this was I'm
> > vary of just pushing this to -fixes without some solid testing.
> >
>
> Hi Daniel,
>
> Thanks for merging, more testing definitely sounds good to me.
>
> > One thing I noticed is that drm_is_current_master could just use the
> > spinlock, since it's only doing a read access. Care to type up that patch?
> >
>
> I thought about this too, but I'm not sure if that's the best solution.
>
> drm_is_current_master calls drm_lease_owner which then walks up the tree
> of master lessors. The spinlock protects the master of the current drm
> file, but subsequent lessors aren't protected without holding the
> device's master mutex.
But this isn't a fpriv->master pointer, but a master->lessor pointer.
Which should never ever be able to change (we'd have tons of uaf bugs
around drm_lease_owner otherwise). So I don't think there's anything
that dev->master_lock protects here that fpriv->master_lookup_lock
doesn't protect already?
Or am I missing something?
The comment in the struct drm_master says it's protected by
mode_config.idr_mutex, but that only applies to the idrs and lists I
think.
> > Also, do you plan to look into that idea we've discussed to flush pending
> > access when we revoke a master or a lease? I think that would be really
> > nice improvement here.
> > -Daniel
> >
>
> Yup, now that the potential UAFs are addressed (hopefully), I'll take a
> closer look and propose a patch for this.
Thanks a lot.
-Daniel
>
> Best wishes,
> Desmond
>
> >>
> >> Desmond Cheong Zhi Xi (5):
> >> drm: avoid circular locks in drm_mode_getconnector
> >> drm: avoid blocking in drm_clients_info's rcu section
> >> drm: add a locked version of drm_is_current_master
> >> drm: serialize drm_file.master with a new spinlock
> >> drm: protect drm_master pointers in drm_lease.c
> >>
> >> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++---------
> >> drivers/gpu/drm/drm_connector.c | 5 +-
> >> drivers/gpu/drm/drm_debugfs.c | 3 +-
> >> drivers/gpu/drm/drm_file.c | 1 +
> >> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++-------
> >> include/drm/drm_auth.h | 1 +
> >> include/drm/drm_file.h | 18 +++++--
> >> 7 files changed, 152 insertions(+), 50 deletions(-)
> >>
> >> --
> >> 2.25.1
> >>
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 21/7/21 6:29 pm, Daniel Vetter wrote:
> On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi
> <[email protected]> wrote:
>> On 21/7/21 2:24 am, Daniel Vetter wrote:
>>> On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote:
>>>> Hi,
>>>>
>>>> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this.
>>>>
>>>> Overall, this 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 five 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. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info().
>>>>
>>>> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
>>>>
>>>> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes.
>>>>
>>>> 5. 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.
>>>>
>>>> v7 -> v8:
>>>> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI.
>>>> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI.
>>>>
>>>> v6 -> v7:
>>>> - Modify code alignment as suggested by the intel-gfx CI.
>>>> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI.
>>>> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI.
>>>>
>>>> v5 -> v6:
>>>> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find.
>>>> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter.
>>>> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov.
>>>> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI.
>>>>
>>>> v4 -> v5:
>>>> - Add a new patch to the series that 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.
>>>>
>>>> v3 -> v4:
>>>> - 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/
>>>> - 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.
>>>> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter.
>>>> - 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.
>>>>
>>>> v2 -> v3:
>>>> - 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.
>>>> - 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.
>>>>
>>>> v1 -> v2:
>>>> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov.
>>>
>>> Apologies for the delay, I missed your series. Maybe just ping next time
>>> around there's silence.
>>>
>>> Looks all great, merged to drm-misc-next. Given how complex this was I'm
>>> vary of just pushing this to -fixes without some solid testing.
>>>
>>
>> Hi Daniel,
>>
>> Thanks for merging, more testing definitely sounds good to me.
>>
>>> One thing I noticed is that drm_is_current_master could just use the
>>> spinlock, since it's only doing a read access. Care to type up that patch?
>>>
>>
>> I thought about this too, but I'm not sure if that's the best solution.
>>
>> drm_is_current_master calls drm_lease_owner which then walks up the tree
>> of master lessors. The spinlock protects the master of the current drm
>> file, but subsequent lessors aren't protected without holding the
>> device's master mutex.
>
> But this isn't a fpriv->master pointer, but a master->lessor pointer.
> Which should never ever be able to change (we'd have tons of uaf bugs
> around drm_lease_owner otherwise). So I don't think there's anything
> that dev->master_lock protects here that fpriv->master_lookup_lock
> doesn't protect already?
>
> Or am I missing something?
> > The comment in the struct drm_master says it's protected by
> mode_config.idr_mutex, but that only applies to the idrs and lists I
> think.
>
Ah you're right, I also completely forgot that lessees hold a reference
to their lessor so nothing will be freed as long as the spinlock is
held. I'll prepare that patch then, thanks for pointing it out.
>>> Also, do you plan to look into that idea we've discussed to flush pending
>>> access when we revoke a master or a lease? I think that would be really
>>> nice improvement here.
>>> -Daniel
>>>
>>
>> Yup, now that the potential UAFs are addressed (hopefully), I'll take a
>> closer look and propose a patch for this.
>
> Thanks a lot.
> -Daniel
>
>>
>> Best wishes,
>> Desmond
>>
>>>>
>>>> Desmond Cheong Zhi Xi (5):
>>>> drm: avoid circular locks in drm_mode_getconnector
>>>> drm: avoid blocking in drm_clients_info's rcu section
>>>> drm: add a locked version of drm_is_current_master
>>>> drm: serialize drm_file.master with a new spinlock
>>>> drm: protect drm_master pointers in drm_lease.c
>>>>
>>>> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++---------
>>>> drivers/gpu/drm/drm_connector.c | 5 +-
>>>> drivers/gpu/drm/drm_debugfs.c | 3 +-
>>>> drivers/gpu/drm/drm_file.c | 1 +
>>>> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++-------
>>>> include/drm/drm_auth.h | 1 +
>>>> include/drm/drm_file.h | 18 +++++--
>>>> 7 files changed, 152 insertions(+), 50 deletions(-)
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>
>>
>
>
On Wed, Jul 21, 2021 at 2:44 PM Desmond Cheong Zhi Xi
<[email protected]> wrote:
> On 21/7/21 6:29 pm, Daniel Vetter wrote:
> > On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi
> > <[email protected]> wrote:
> >> On 21/7/21 2:24 am, Daniel Vetter wrote:
> >>> On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote:
> >>>> Hi,
> >>>>
> >>>> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this.
> >>>>
> >>>> Overall, this 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 five 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. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info().
> >>>>
> >>>> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
> >>>>
> >>>> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes.
> >>>>
> >>>> 5. 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.
> >>>>
> >>>> v7 -> v8:
> >>>> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI.
> >>>> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI.
> >>>>
> >>>> v6 -> v7:
> >>>> - Modify code alignment as suggested by the intel-gfx CI.
> >>>> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI.
> >>>> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI.
> >>>>
> >>>> v5 -> v6:
> >>>> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find.
> >>>> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter.
> >>>> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov.
> >>>> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI.
> >>>>
> >>>> v4 -> v5:
> >>>> - Add a new patch to the series that 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.
> >>>>
> >>>> v3 -> v4:
> >>>> - 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/
> >>>> - 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.
> >>>> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter.
> >>>> - 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.
> >>>>
> >>>> v2 -> v3:
> >>>> - 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.
> >>>> - 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.
> >>>>
> >>>> v1 -> v2:
> >>>> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov.
> >>>
> >>> Apologies for the delay, I missed your series. Maybe just ping next time
> >>> around there's silence.
> >>>
> >>> Looks all great, merged to drm-misc-next. Given how complex this was I'm
> >>> vary of just pushing this to -fixes without some solid testing.
> >>>
> >>
> >> Hi Daniel,
> >>
> >> Thanks for merging, more testing definitely sounds good to me.
> >>
> >>> One thing I noticed is that drm_is_current_master could just use the
> >>> spinlock, since it's only doing a read access. Care to type up that patch?
> >>>
> >>
> >> I thought about this too, but I'm not sure if that's the best solution.
> >>
> >> drm_is_current_master calls drm_lease_owner which then walks up the tree
> >> of master lessors. The spinlock protects the master of the current drm
> >> file, but subsequent lessors aren't protected without holding the
> >> device's master mutex.
> >
> > But this isn't a fpriv->master pointer, but a master->lessor pointer.
> > Which should never ever be able to change (we'd have tons of uaf bugs
> > around drm_lease_owner otherwise). So I don't think there's anything
> > that dev->master_lock protects here that fpriv->master_lookup_lock
> > doesn't protect already?
> >
> > Or am I missing something?
> > > The comment in the struct drm_master says it's protected by
> > mode_config.idr_mutex, but that only applies to the idrs and lists I
> > think.
> >
>
> Ah you're right, I also completely forgot that lessees hold a reference
> to their lessor so nothing will be freed as long as the spinlock is
> held. I'll prepare that patch then, thanks for pointing it out.
btw since we now looked at all this in detail, can you perhaps do a
patch to update the kerneldoc for all the lease fields in struct
drm_master? I think moving them to the inline style and then adding
comments for each field how locking/lifetime rules work would be
really good. Since right now it's all fresh from for us.
-Daniel
> >>> Also, do you plan to look into that idea we've discussed to flush pending
> >>> access when we revoke a master or a lease? I think that would be really
> >>> nice improvement here.
> >>> -Daniel
> >>>
> >>
> >> Yup, now that the potential UAFs are addressed (hopefully), I'll take a
> >> closer look and propose a patch for this.
> >
> > Thanks a lot.
> > -Daniel
> >
> >>
> >> Best wishes,
> >> Desmond
> >>
> >>>>
> >>>> Desmond Cheong Zhi Xi (5):
> >>>> drm: avoid circular locks in drm_mode_getconnector
> >>>> drm: avoid blocking in drm_clients_info's rcu section
> >>>> drm: add a locked version of drm_is_current_master
> >>>> drm: serialize drm_file.master with a new spinlock
> >>>> drm: protect drm_master pointers in drm_lease.c
> >>>>
> >>>> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++---------
> >>>> drivers/gpu/drm/drm_connector.c | 5 +-
> >>>> drivers/gpu/drm/drm_debugfs.c | 3 +-
> >>>> drivers/gpu/drm/drm_file.c | 1 +
> >>>> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++-------
> >>>> include/drm/drm_auth.h | 1 +
> >>>> include/drm/drm_file.h | 18 +++++--
> >>>> 7 files changed, 152 insertions(+), 50 deletions(-)
> >>>>
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>
> >>
> >
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 21/7/21 9:23 pm, Daniel Vetter wrote:
> On Wed, Jul 21, 2021 at 2:44 PM Desmond Cheong Zhi Xi
> <[email protected]> wrote:
>> On 21/7/21 6:29 pm, Daniel Vetter wrote:
>>> On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi
>>> <[email protected]> wrote:
>>>> On 21/7/21 2:24 am, Daniel Vetter wrote:
>>>>> On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this.
>>>>>>
>>>>>> Overall, this 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 five 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. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info().
>>>>>>
>>>>>> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
>>>>>>
>>>>>> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes.
>>>>>>
>>>>>> 5. 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.
>>>>>>
>>>>>> v7 -> v8:
>>>>>> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI.
>>>>>> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI.
>>>>>>
>>>>>> v6 -> v7:
>>>>>> - Modify code alignment as suggested by the intel-gfx CI.
>>>>>> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI.
>>>>>> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI.
>>>>>>
>>>>>> v5 -> v6:
>>>>>> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find.
>>>>>> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter.
>>>>>> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov.
>>>>>> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI.
>>>>>>
>>>>>> v4 -> v5:
>>>>>> - Add a new patch to the series that 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.
>>>>>>
>>>>>> v3 -> v4:
>>>>>> - 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/
>>>>>> - 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.
>>>>>> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter.
>>>>>> - 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.
>>>>>>
>>>>>> v2 -> v3:
>>>>>> - 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.
>>>>>> - 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.
>>>>>>
>>>>>> v1 -> v2:
>>>>>> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov.
>>>>>
>>>>> Apologies for the delay, I missed your series. Maybe just ping next time
>>>>> around there's silence.
>>>>>
>>>>> Looks all great, merged to drm-misc-next. Given how complex this was I'm
>>>>> vary of just pushing this to -fixes without some solid testing.
>>>>>
>>>>
>>>> Hi Daniel,
>>>>
>>>> Thanks for merging, more testing definitely sounds good to me.
>>>>
>>>>> One thing I noticed is that drm_is_current_master could just use the
>>>>> spinlock, since it's only doing a read access. Care to type up that patch?
>>>>>
>>>>
>>>> I thought about this too, but I'm not sure if that's the best solution.
>>>>
>>>> drm_is_current_master calls drm_lease_owner which then walks up the tree
>>>> of master lessors. The spinlock protects the master of the current drm
>>>> file, but subsequent lessors aren't protected without holding the
>>>> device's master mutex.
>>>
>>> But this isn't a fpriv->master pointer, but a master->lessor pointer.
>>> Which should never ever be able to change (we'd have tons of uaf bugs
>>> around drm_lease_owner otherwise). So I don't think there's anything
>>> that dev->master_lock protects here that fpriv->master_lookup_lock
>>> doesn't protect already?
>>>
>>> Or am I missing something?
>>> > The comment in the struct drm_master says it's protected by
>>> mode_config.idr_mutex, but that only applies to the idrs and lists I
>>> think.
>>>
>>
>> Ah you're right, I also completely forgot that lessees hold a reference
>> to their lessor so nothing will be freed as long as the spinlock is
>> held. I'll prepare that patch then, thanks for pointing it out.
>
> btw since we now looked at all this in detail, can you perhaps do a
> patch to update the kerneldoc for all the lease fields in struct
> drm_master? I think moving them to the inline style and then adding
> comments for each field how locking/lifetime rules work would be
> really good. Since right now it's all fresh from for us.
> -Daniel
>
Sure thing. Just sent out the suggested changes in the same series,
along with a relevant fix for drm/vmwgfx that I just noticed.
>>>>> Also, do you plan to look into that idea we've discussed to flush pending
>>>>> access when we revoke a master or a lease? I think that would be really
>>>>> nice improvement here.
>>>>> -Daniel
>>>>>
>>>>
>>>> Yup, now that the potential UAFs are addressed (hopefully), I'll take a
>>>> closer look and propose a patch for this.
>>>
>>> Thanks a lot.
>>> -Daniel
>>>
>>>>
>>>> Best wishes,
>>>> Desmond
>>>>
>>>>>>
>>>>>> Desmond Cheong Zhi Xi (5):
>>>>>> drm: avoid circular locks in drm_mode_getconnector
>>>>>> drm: avoid blocking in drm_clients_info's rcu section
>>>>>> drm: add a locked version of drm_is_current_master
>>>>>> drm: serialize drm_file.master with a new spinlock
>>>>>> drm: protect drm_master pointers in drm_lease.c
>>>>>>
>>>>>> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++---------
>>>>>> drivers/gpu/drm/drm_connector.c | 5 +-
>>>>>> drivers/gpu/drm/drm_debugfs.c | 3 +-
>>>>>> drivers/gpu/drm/drm_file.c | 1 +
>>>>>> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++-------
>>>>>> include/drm/drm_auth.h | 1 +
>>>>>> include/drm/drm_file.h | 18 +++++--
>>>>>> 7 files changed, 152 insertions(+), 50 deletions(-)
>>>>>>
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>