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 two patches:
1. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c.
2. 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 v2 -> v3:
- Patch 1: 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 2: 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 2: 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 (2):
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 | 73 +++++++++++++++++++++++++++----------
drivers/gpu/drm/drm_lease.c | 57 ++++++++++++++++++++---------
include/drm/drm_auth.h | 1 +
include/drm/drm_file.h | 15 ++++++--
4 files changed, 107 insertions(+), 39 deletions(-)
--
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 232abbba3686..86d4b72e95cb 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->master->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->master->dev->master_mutex);
+ ret = drm_is_current_master_locked(fpriv);
+ mutex_unlock(&fpriv->master->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, 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 as follows:
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 <[email protected]>
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
drivers/gpu/drm/drm_auth.c | 22 ++++++++++++++
drivers/gpu/drm/drm_lease.c | 57 ++++++++++++++++++++++++++-----------
include/drm/drm_auth.h | 1 +
include/drm/drm_file.h | 15 ++++++++--
4 files changed, 75 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 86d4b72e95cb..0c64a77c67a6 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -384,6 +384,28 @@ struct drm_master *drm_master_get(struct drm_master *master)
}
EXPORT_SYMBOL(drm_master_get);
+/**
+ * drm_file_get_master - reference @file_priv->master
+ * @file_priv: DRM file private
+ *
+ * Increments the reference count of @file_priv->master and returns
+ * @file_priv->master.
+ *
+ * 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;
+
+ mutex_lock(&file_priv->master->dev->master_mutex);
+ master = drm_master_get(file_priv->master);
+ mutex_unlock(&file_priv->master->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 da4f085fc09e..65eab82f8acc 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -107,10 +107,17 @@ static bool _drm_has_leased(struct drm_master *master, int id)
*/
bool _drm_lease_held(struct drm_file *file_priv, int id)
{
+ bool ret;
+ struct drm_master *master;
+
if (!file_priv || !file_priv->master)
return true;
- return _drm_lease_held_master(file_priv->master, id);
+ master = drm_file_get_master(file_priv);
+ ret = _drm_lease_held_master(master, id);
+ drm_master_put(&master);
+
+ return ret;
}
/**
@@ -132,10 +139,11 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
if (!file_priv || !file_priv->master || !file_priv->master->lessor)
return true;
- master = file_priv->master;
+ master = drm_file_get_master(file_priv);
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;
}
@@ -158,7 +166,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
if (!file_priv || !file_priv->master || !file_priv->master->lessor)
return crtcs_in;
- master = file_priv->master;
+ master = drm_file_get_master(file_priv);
dev = master->dev;
count_in = count_out = 0;
@@ -177,6 +185,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;
}
@@ -490,7 +499,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;
@@ -502,12 +511,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");
@@ -519,12 +522,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);
@@ -535,14 +548,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");
@@ -578,6 +592,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;
@@ -587,6 +602,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;
}
@@ -609,7 +626,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;
@@ -620,6 +637,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);
@@ -643,6 +661,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;
}
@@ -662,7 +681,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;
@@ -676,6 +695,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);
@@ -703,6 +723,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;
}
@@ -721,7 +742,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;
@@ -731,6 +752,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);
@@ -751,6 +773,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
On Sun, Jun 20, 2021 at 07:03:26PM +0800, Desmond Cheong Zhi Xi wrote:
> 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]>
Merged to drm-misc-fixes, thanks for your patch.
-Daniel
> ---
> 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 232abbba3686..86d4b72e95cb 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->master->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->master->dev->master_mutex);
> + ret = drm_is_current_master_locked(fpriv);
> + mutex_unlock(&fpriv->master->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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Sun, Jun 20, 2021 at 07:03:27PM +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 as follows:
>
> 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 <[email protected]>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
I think this approach looks much clearer than the previous. I've found a
few smaller things below still. I think at least ...
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_auth.c | 22 ++++++++++++++
> drivers/gpu/drm/drm_lease.c | 57 ++++++++++++++++++++++++++-----------
> include/drm/drm_auth.h | 1 +
> include/drm/drm_file.h | 15 ++++++++--
> 4 files changed, 75 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 86d4b72e95cb..0c64a77c67a6 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -384,6 +384,28 @@ struct drm_master *drm_master_get(struct drm_master *master)
> }
> EXPORT_SYMBOL(drm_master_get);
>
> +/**
> + * drm_file_get_master - reference @file_priv->master
> + * @file_priv: DRM file private
> + *
> + * Increments the reference count of @file_priv->master and returns
Does this format correctly? I'd go with "&drm_file.master of @file_priv".
> + * @file_priv->master.
> + *
> + * 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;
> +
> + mutex_lock(&file_priv->master->dev->master_mutex);
> + master = drm_master_get(file_priv->master);
> + mutex_unlock(&file_priv->master->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 da4f085fc09e..65eab82f8acc 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -107,10 +107,17 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> */
> bool _drm_lease_held(struct drm_file *file_priv, int id)
> {
> + bool ret;
> + struct drm_master *master;
> +
> if (!file_priv || !file_priv->master)
So here we still have a ->master access outside of the locked code
section. I think the best fix for that would be to move the NULL check
into drm_file_get_master (where we grab the lock already anyway), and
update the kerneldoc to state that it might return NULL.
Same with all the checks for ->master below.
> return true;
>
> - return _drm_lease_held_master(file_priv->master, id);
> + master = drm_file_get_master(file_priv);
> + ret = _drm_lease_held_master(master, id);
> + drm_master_put(&master);
> +
> + return ret;
> }
>
> /**
> @@ -132,10 +139,11 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
> if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> return true;
master->lessor dereferenced outside the lock or without holding a
reference.
>
> - master = file_priv->master;
> + master = drm_file_get_master(file_priv);
> 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;
> }
>
> @@ -158,7 +166,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> return crtcs_in;
Same here.
>
> - master = file_priv->master;
> + master = drm_file_get_master(file_priv);
> dev = master->dev;
>
> count_in = count_out = 0;
> @@ -177,6 +185,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;
> }
>
> @@ -490,7 +499,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;
> @@ -502,12 +511,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");
> @@ -519,12 +522,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) {
Here we check after grabbing the reference, so looks correct.
> + 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);
>
> @@ -535,14 +548,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");
> @@ -578,6 +592,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;
>
> @@ -587,6 +602,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;
> }
> @@ -609,7 +626,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;
>
> @@ -620,6 +637,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);
> @@ -643,6 +661,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;
> }
> @@ -662,7 +681,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;
> @@ -676,6 +695,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);
> @@ -703,6 +723,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;
> }
> @@ -721,7 +742,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;
>
> @@ -731,6 +752,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);
> @@ -751,6 +773,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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 21/6/21 10:47 pm, Daniel Vetter wrote:
> On Sun, Jun 20, 2021 at 07:03:27PM +0800, Desmond Cheong Zhi Xi wrote:
>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>> index 86d4b72e95cb..0c64a77c67a6 100644
>> --- a/drivers/gpu/drm/drm_auth.c
>> +++ b/drivers/gpu/drm/drm_auth.c
>> @@ -384,6 +384,28 @@ struct drm_master *drm_master_get(struct drm_master *master)
>> }
>> EXPORT_SYMBOL(drm_master_get);
>>
>> +/**
>> + * drm_file_get_master - reference @file_priv->master
>> + * @file_priv: DRM file private
>> + *
>> + * Increments the reference count of @file_priv->master and returns
>
> Does this format correctly? I'd go with "&drm_file.master of @file_priv".
>
Got it. "file_priv->master" was bolded, but no link to drm_file.master
was generated. I'll update this.
>> + * @file_priv->master.
>> + *
>> + * 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;
>> +
>> + mutex_lock(&file_priv->master->dev->master_mutex);
>> + master = drm_master_get(file_priv->master);
>> + mutex_unlock(&file_priv->master->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 da4f085fc09e..65eab82f8acc 100644
>> --- a/drivers/gpu/drm/drm_lease.c
>> +++ b/drivers/gpu/drm/drm_lease.c
>> @@ -107,10 +107,17 @@ static bool _drm_has_leased(struct drm_master *master, int id)
>> */
>> bool _drm_lease_held(struct drm_file *file_priv, int id)
>> {
>> + bool ret;
>> + struct drm_master *master;
>> +
>> if (!file_priv || !file_priv->master)
>
> So here we still have a ->master access outside of the locked code
> section. I think the best fix for that would be to move the NULL check
> into drm_file_get_master (where we grab the lock already anyway), and
> update the kerneldoc to state that it might return NULL.
>
> Same with all the checks for ->master below.
>
Moving the check into drm_file_get_master sounds good. Grabbing the lock
before performing the NULL check poses a little chicken-and-egg problem
though.
It's true that without the lock, even if file_priv->master passes the
NULL check, it could be freed in the time between the check and grabbing
the lock.
However, based on the original code, it seems there's the possibility
that file_priv->master might be NULL. In this case, grabbing the lock
results in a null ptr dereference because we get the mutex via
&file_priv->master->dev->master_mutex.
By this reasoning, I think the safer method is still to perform the NULL
check before grabbing the lock.
>> return true;
>>
>> - return _drm_lease_held_master(file_priv->master, id);
>> + master = drm_file_get_master(file_priv);
>> + ret = _drm_lease_held_master(master, id);
>> + drm_master_put(&master);
>> +
>> + return ret;
>> }
>>
>> /**
>> @@ -132,10 +139,11 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>> if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>> return true;
>
> master->lessor dereferenced outside the lock or without holding a
> reference.
>
>>
>> - master = file_priv->master;
>> + master = drm_file_get_master(file_priv);
>> 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;
>> }
>>
>> @@ -158,7 +166,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>> if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>> return crtcs_in;
>
> Same here.
>
>>
>> - master = file_priv->master;
>> + master = drm_file_get_master(file_priv);
>> dev = master->dev;
>>
>> count_in = count_out = 0;
>> @@ -177,6 +185,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;
>> }
>>
>> @@ -490,7 +499,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;
>> @@ -502,12 +511,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");
>> @@ -519,12 +522,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) {
>
> Here we check after grabbing the reference, so looks correct.
>
>> + 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);
>>
>> @@ -535,14 +548,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");
>> @@ -578,6 +592,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;
>>
>> @@ -587,6 +602,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;
>> }
>> @@ -609,7 +626,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;
>>
>> @@ -620,6 +637,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);
>> @@ -643,6 +661,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;
>> }
>> @@ -662,7 +681,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;
>> @@ -676,6 +695,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);
>> @@ -703,6 +723,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;
>> }
>> @@ -721,7 +742,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;
>>
>> @@ -731,6 +752,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);
>> @@ -751,6 +773,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
>>
>
Thanks for the feedback, Daniel. I'll send out an updated patch to
address these issues.
Best wishes,
Desmond
On Wed, Jun 23, 2021 at 5:49 AM Desmond Cheong Zhi Xi
<[email protected]> wrote:
>
> On 21/6/21 10:47 pm, Daniel Vetter wrote:
> > On Sun, Jun 20, 2021 at 07:03:27PM +0800, Desmond Cheong Zhi Xi wrote:
> >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> >> index 86d4b72e95cb..0c64a77c67a6 100644
> >> --- a/drivers/gpu/drm/drm_auth.c
> >> +++ b/drivers/gpu/drm/drm_auth.c
> >> @@ -384,6 +384,28 @@ struct drm_master *drm_master_get(struct drm_master *master)
> >> }
> >> EXPORT_SYMBOL(drm_master_get);
> >>
> >> +/**
> >> + * drm_file_get_master - reference @file_priv->master
> >> + * @file_priv: DRM file private
> >> + *
> >> + * Increments the reference count of @file_priv->master and returns
> >
> > Does this format correctly? I'd go with "&drm_file.master of @file_priv".
> >
>
> Got it. "file_priv->master" was bolded, but no link to drm_file.master
> was generated. I'll update this.
>
> >> + * @file_priv->master.
> >> + *
> >> + * 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;
> >> +
> >> + mutex_lock(&file_priv->master->dev->master_mutex);
> >> + master = drm_master_get(file_priv->master);
> >> + mutex_unlock(&file_priv->master->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 da4f085fc09e..65eab82f8acc 100644
> >> --- a/drivers/gpu/drm/drm_lease.c
> >> +++ b/drivers/gpu/drm/drm_lease.c
> >> @@ -107,10 +107,17 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> >> */
> >> bool _drm_lease_held(struct drm_file *file_priv, int id)
> >> {
> >> + bool ret;
> >> + struct drm_master *master;
> >> +
> >> if (!file_priv || !file_priv->master)
> >
> > So here we still have a ->master access outside of the locked code
> > section. I think the best fix for that would be to move the NULL check
> > into drm_file_get_master (where we grab the lock already anyway), and
> > update the kerneldoc to state that it might return NULL.
> >
> > Same with all the checks for ->master below.
> >
>
> Moving the check into drm_file_get_master sounds good. Grabbing the lock
> before performing the NULL check poses a little chicken-and-egg problem
> though.
>
> It's true that without the lock, even if file_priv->master passes the
> NULL check, it could be freed in the time between the check and grabbing
> the lock.
>
> However, based on the original code, it seems there's the possibility
> that file_priv->master might be NULL. In this case, grabbing the lock
> results in a null ptr dereference because we get the mutex via
> &file_priv->master->dev->master_mutex.
>
> By this reasoning, I think the safer method is still to perform the NULL
> check before grabbing the lock.
file_priv->dev->master_mutex should also work and avoid the trouble.
Please also cc intel-gfx list, there's a CI system there to test your
patches. Since patch 1 of this series had pretty bad deadlock that I
didn't see would be good to make sure we get more test coverage on
these.
Thanks, Daniel
>
> >> return true;
> >>
> >> - return _drm_lease_held_master(file_priv->master, id);
> >> + master = drm_file_get_master(file_priv);
> >> + ret = _drm_lease_held_master(master, id);
> >> + drm_master_put(&master);
> >> +
> >> + return ret;
> >> }
> >>
> >> /**
> >> @@ -132,10 +139,11 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
> >> if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >> return true;
> >
> > master->lessor dereferenced outside the lock or without holding a
> > reference.
> >
> >>
> >> - master = file_priv->master;
> >> + master = drm_file_get_master(file_priv);
> >> 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;
> >> }
> >>
> >> @@ -158,7 +166,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> >> if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >> return crtcs_in;
> >
> > Same here.
> >
> >>
> >> - master = file_priv->master;
> >> + master = drm_file_get_master(file_priv);
> >> dev = master->dev;
> >>
> >> count_in = count_out = 0;
> >> @@ -177,6 +185,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;
> >> }
> >>
> >> @@ -490,7 +499,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;
> >> @@ -502,12 +511,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");
> >> @@ -519,12 +522,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) {
> >
> > Here we check after grabbing the reference, so looks correct.
> >
> >> + 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);
> >>
> >> @@ -535,14 +548,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");
> >> @@ -578,6 +592,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;
> >>
> >> @@ -587,6 +602,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;
> >> }
> >> @@ -609,7 +626,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;
> >>
> >> @@ -620,6 +637,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);
> >> @@ -643,6 +661,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;
> >> }
> >> @@ -662,7 +681,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;
> >> @@ -676,6 +695,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);
> >> @@ -703,6 +723,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;
> >> }
> >> @@ -721,7 +742,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;
> >>
> >> @@ -731,6 +752,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);
> >> @@ -751,6 +773,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
> >>
> >
>
> Thanks for the feedback, Daniel. I'll send out an updated patch to
> address these issues.
>
> Best wishes,
> Desmond
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Jun 21, 2021 at 4:25 PM Daniel Vetter <[email protected]> wrote:
>
> On Sun, Jun 20, 2021 at 07:03:26PM +0800, Desmond Cheong Zhi Xi wrote:
> > 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]>
>
> Merged to drm-misc-fixes, thanks for your patch.
Cc'ed you on the revert, but this blew up in intel-gfx CI. Please cc:
[email protected] for the next round so CI can pick it
up (it doesn't read dri-devel here).
I'm not exactly sure how we can best fix that issue in general, maybe
there's more. But for the specific lockdep splat around getconnector I
think just pulling the call to drm_is_current_master out from the
connector mutex should avoid the issue (just store it locally and then
still have the if() condition under the connector mutex ofc).
-Daniel
> -Daniel
>
> > ---
> > 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 232abbba3686..86d4b72e95cb 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->master->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->master->dev->master_mutex);
> > + ret = drm_is_current_master_locked(fpriv);
> > + mutex_unlock(&fpriv->master->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
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch