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=148d2f1dfac64af52ffd27b661981a540724f80...
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 protected by a mutex
Change in 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 | 23 ++++++++++++--- drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++---------- 2 files changed, 62 insertions(+), 19 deletions(-)
While checking the master status of the DRM file in drm_is_current_master(), the device's master mutex should be held. Without the mutex, the pointer fpriv->master may be freed concurrently by another process calling drm_setmaster_ioctl(). This could lead to use-after-free errors when the pointer is subsequently dereferenced in drm_lease_owner().
The callers of drm_is_current_master() from drm_auth.c hold the device's master mutex, but external callers do not. Hence, we implement drm_is_current_master_locked() to be used within drm_auth.c, and modify drm_is_current_master() to grab the device's master mutex before checking the master status.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com --- drivers/gpu/drm/drm_auth.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 232abbba3686..c6bf52c310a9 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -61,6 +61,8 @@ * trusted clients. */
+static bool drm_is_current_master_locked(struct drm_file *fpriv); + int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_auth *auth = data; @@ -223,7 +225,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 +274,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 +323,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,6 +344,13 @@ void drm_master_release(struct drm_file *file_priv) mutex_unlock(&dev->master_mutex); }
+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 @@ -354,7 +363,13 @@ void drm_master_release(struct drm_file *file_priv) */ bool drm_is_current_master(struct drm_file *fpriv) { - return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; + 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);
On Tue, Jun 15, 2021 at 10:36:44AM +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 daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
drivers/gpu/drm/drm_auth.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 232abbba3686..c6bf52c310a9 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -61,6 +61,8 @@
- trusted clients.
*/
+static bool drm_is_current_master_locked(struct drm_file *fpriv);
A bit a bikeshed, but we try to avoid forward declarations when they're not needed. If you don't want to tear apart drm_is_current_master and the _locked version then just move them together.
Can you pls do that and respin?
Otherwise looks all great. -Daniel
int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_auth *auth = data; @@ -223,7 +225,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 +274,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 +323,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,6 +344,13 @@ void drm_master_release(struct drm_file *file_priv) mutex_unlock(&dev->master_mutex); }
+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
@@ -354,7 +363,13 @@ void drm_master_release(struct drm_file *file_priv) */ bool drm_is_current_master(struct drm_file *fpriv) {
- return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
- 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);
-- 2.25.1
On 18/6/21 1:03 am, Daniel Vetter wrote:
On Tue, Jun 15, 2021 at 10:36:44AM +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 daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
drivers/gpu/drm/drm_auth.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 232abbba3686..c6bf52c310a9 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -61,6 +61,8 @@
- trusted clients.
*/
+static bool drm_is_current_master_locked(struct drm_file *fpriv);
A bit a bikeshed, but we try to avoid forward declarations when they're not needed. If you don't want to tear apart drm_is_current_master and the _locked version then just move them together.
Can you pls do that and respin?
Otherwise looks all great. -Daniel
Yeah, I was trying to keep the logic in _locked close to drm_is_current_master. But got it, will do.
This patch ensures that the device's master mutex is acquired before accessing pointers to struct drm_master that are subsequently dereferenced. Without the mutex, the struct drm_master may be freed concurrently by another process calling drm_setmaster_ioctl(). This could then lead to use-after-free errors.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com --- drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index da4f085fc09e..3e6f689236e5 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -107,10 +107,16 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) { + bool ret; + if (!file_priv || !file_priv->master) return true;
- return _drm_lease_held_master(file_priv->master, id); + mutex_lock(&file_priv->master->dev->master_mutex); + ret = _drm_lease_held_master(file_priv->master, id); + mutex_unlock(&file_priv->master->dev->master_mutex); + + return ret; }
/** @@ -132,10 +138,12 @@ bool drm_lease_held(struct drm_file *file_priv, int id) if (!file_priv || !file_priv->master || !file_priv->master->lessor) return true;
+ mutex_lock(&file_priv->master->dev->master_mutex); master = file_priv->master; mutex_lock(&master->dev->mode_config.idr_mutex); ret = _drm_lease_held_master(master, id); mutex_unlock(&master->dev->mode_config.idr_mutex); + mutex_unlock(&file_priv->master->dev->master_mutex); return ret; }
@@ -158,6 +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;
+ mutex_lock(&file_priv->master->dev->master_mutex); master = file_priv->master; dev = master->dev;
@@ -177,6 +186,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); + mutex_unlock(&file_priv->master->dev->master_mutex); return crtcs_out; }
@@ -490,7 +500,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 +512,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 +523,23 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, return -EINVAL; }
+ mutex_lock(&dev->master_mutex); + lessor = lessor_priv->master; + /* Do not allow sub-leases */ + if (lessor->lessor) { + DRM_DEBUG_LEASE("recursive leasing not allowed\n"); + ret = -EINVAL; + goto unlock; + } + 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 unlock; + }
idr_init(&leases);
@@ -535,14 +550,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 unlock; }
/* 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 unlock; }
DRM_DEBUG_LEASE("Creating lease\n"); @@ -578,6 +594,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, /* Hook up the fd */ fd_install(fd, lessee_file);
+ mutex_unlock(&dev->master_mutex); DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); return 0;
@@ -587,6 +604,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, out_leases: put_unused_fd(fd);
+unlock: + mutex_unlock(&dev->master_mutex); DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); return ret; } @@ -609,7 +628,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 +639,8 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
+ mutex_lock(&dev->master_mutex); + lessor = lessor_priv->master; DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
mutex_lock(&dev->mode_config.idr_mutex); @@ -643,6 +664,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, arg->count_lessees = count;
mutex_unlock(&dev->mode_config.idr_mutex); + mutex_unlock(&dev->master_mutex);
return ret; } @@ -662,7 +684,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 +698,8 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
+ mutex_lock(&dev->master_mutex); + lessee = lessee_priv->master; DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
mutex_lock(&dev->mode_config.idr_mutex); @@ -703,6 +727,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, arg->count_objects = count;
mutex_unlock(&dev->mode_config.idr_mutex); + mutex_unlock(&dev->master_mutex);
return ret; } @@ -721,7 +746,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,8 +756,10 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
+ mutex_lock(&dev->master_mutex); mutex_lock(&dev->mode_config.idr_mutex);
+ lessor = lessor_priv->master; lessee = _drm_find_lessee(lessor, arg->lessee_id);
/* No such lessee */ @@ -751,6 +778,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
fail: mutex_unlock(&dev->mode_config.idr_mutex); + mutex_unlock(&dev->master_mutex);
return ret; }
On Tue, Jun 15, 2021 at 10:36:45AM +0800, Desmond Cheong Zhi Xi wrote:
This patch ensures that the device's master mutex is acquired before accessing pointers to struct drm_master that are subsequently dereferenced. Without the mutex, the struct drm_master may be freed concurrently by another process calling drm_setmaster_ioctl(). This could then lead to use-after-free errors.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index da4f085fc09e..3e6f689236e5 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -107,10 +107,16 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) {
- bool ret;
- if (!file_priv || !file_priv->master) return true;
- return _drm_lease_held_master(file_priv->master, id);
- mutex_lock(&file_priv->master->dev->master_mutex);
So maybe we have a bug somewhere, and the kerneldoc isn't 100% clear, but I thought file_priv->master is invariant over the lifetime of file_priv. So we don't need a lock to check anything here.
It's the drm_device->master derefence that gets us into trouble. Well also file_priv->is_owner is protected by dev->master_mutex.
So I think with your previous patch all the access here in drm_lease.c is ok and already protected? Or am I missing something?
Thanks, Daniel
- ret = _drm_lease_held_master(file_priv->master, id);
- mutex_unlock(&file_priv->master->dev->master_mutex);
- return ret;
}
/** @@ -132,10 +138,12 @@ bool drm_lease_held(struct drm_file *file_priv, int id) if (!file_priv || !file_priv->master || !file_priv->master->lessor) return true;
- mutex_lock(&file_priv->master->dev->master_mutex); master = file_priv->master; mutex_lock(&master->dev->mode_config.idr_mutex); ret = _drm_lease_held_master(master, id); mutex_unlock(&master->dev->mode_config.idr_mutex);
- mutex_unlock(&file_priv->master->dev->master_mutex); return ret;
}
@@ -158,6 +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;
- mutex_lock(&file_priv->master->dev->master_mutex); master = file_priv->master; dev = master->dev;
@@ -177,6 +186,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);
- mutex_unlock(&file_priv->master->dev->master_mutex); return crtcs_out;
}
@@ -490,7 +500,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 +512,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 +523,23 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, return -EINVAL; }
mutex_lock(&dev->master_mutex);
lessor = lessor_priv->master;
/* Do not allow sub-leases */
if (lessor->lessor) {
DRM_DEBUG_LEASE("recursive leasing not allowed\n");ret = -EINVAL;goto unlock;}
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 unlock;}
idr_init(&leases);
@@ -535,14 +550,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 unlock;}
/* 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 unlock;}
DRM_DEBUG_LEASE("Creating lease\n");
@@ -578,6 +594,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, /* Hook up the fd */ fd_install(fd, lessee_file);
- mutex_unlock(&dev->master_mutex); DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); return 0;
@@ -587,6 +604,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, out_leases: put_unused_fd(fd);
+unlock:
- mutex_unlock(&dev->master_mutex); DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); return ret;
} @@ -609,7 +628,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 +639,8 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
mutex_lock(&dev->master_mutex);
lessor = lessor_priv->master; DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
mutex_lock(&dev->mode_config.idr_mutex);
@@ -643,6 +664,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, arg->count_lessees = count;
mutex_unlock(&dev->mode_config.idr_mutex);
mutex_unlock(&dev->master_mutex);
return ret;
} @@ -662,7 +684,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 +698,8 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
mutex_lock(&dev->master_mutex);
lessee = lessee_priv->master; DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
mutex_lock(&dev->mode_config.idr_mutex);
@@ -703,6 +727,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, arg->count_objects = count;
mutex_unlock(&dev->mode_config.idr_mutex);
mutex_unlock(&dev->master_mutex);
return ret;
} @@ -721,7 +746,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,8 +756,10 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
mutex_lock(&dev->master_mutex); mutex_lock(&dev->mode_config.idr_mutex);
lessor = lessor_priv->master; lessee = _drm_find_lessee(lessor, arg->lessee_id);
/* No such lessee */
@@ -751,6 +778,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
fail: mutex_unlock(&dev->mode_config.idr_mutex);
mutex_unlock(&dev->master_mutex);
return ret;
}
2.25.1
On 18/6/21 1:12 am, Daniel Vetter wrote:
On Tue, Jun 15, 2021 at 10:36:45AM +0800, Desmond Cheong Zhi Xi wrote:
This patch ensures that the device's master mutex is acquired before accessing pointers to struct drm_master that are subsequently dereferenced. Without the mutex, the struct drm_master may be freed concurrently by another process calling drm_setmaster_ioctl(). This could then lead to use-after-free errors.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index da4f085fc09e..3e6f689236e5 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -107,10 +107,16 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) {
- bool ret;
- if (!file_priv || !file_priv->master) return true;
- return _drm_lease_held_master(file_priv->master, id);
- mutex_lock(&file_priv->master->dev->master_mutex);
So maybe we have a bug somewhere, and the kerneldoc isn't 100% clear, but I thought file_priv->master is invariant over the lifetime of file_priv. So we don't need a lock to check anything here.
It's the drm_device->master derefence that gets us into trouble. Well also file_priv->is_owner is protected by dev->master_mutex.
So I think with your previous patch all the access here in drm_lease.c is ok and already protected? Or am I missing something?
Thanks, Daniel
My thinking was that file_priv->master is invariant only if it is the creator of master. If file_priv->is_master is false, then a call to drm_setmaster_ioctl will invoke drm_new_set_master, which then allocates a new master for file_priv, and puts the old master.
This could be an issue in _drm_lease_held_master, because we dereference master to get master->dev, master->lessor, and master->leases.
With the same reasoning, in other parts of drm_lease.c, if there's an access to drm_file->master that's subsequently dereferenced, I added a lock around them.
I could definitely be mistaken on this, so apologies if this scenario doesn't arise.
Best wishes, Desmond
On Fri, Jun 18, 2021 at 5:05 AM Desmond Cheong Zhi Xi desmondcheongzx@gmail.com wrote:
On 18/6/21 1:12 am, Daniel Vetter wrote:
On Tue, Jun 15, 2021 at 10:36:45AM +0800, Desmond Cheong Zhi Xi wrote:
This patch ensures that the device's master mutex is acquired before accessing pointers to struct drm_master that are subsequently dereferenced. Without the mutex, the struct drm_master may be freed concurrently by another process calling drm_setmaster_ioctl(). This could then lead to use-after-free errors.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index da4f085fc09e..3e6f689236e5 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -107,10 +107,16 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) {
- bool ret;
- if (!file_priv || !file_priv->master) return true;
- return _drm_lease_held_master(file_priv->master, id);
- mutex_lock(&file_priv->master->dev->master_mutex);
So maybe we have a bug somewhere, and the kerneldoc isn't 100% clear, but I thought file_priv->master is invariant over the lifetime of file_priv. So we don't need a lock to check anything here.
It's the drm_device->master derefence that gets us into trouble. Well also file_priv->is_owner is protected by dev->master_mutex.
So I think with your previous patch all the access here in drm_lease.c is ok and already protected? Or am I missing something?
Thanks, Daniel
My thinking was that file_priv->master is invariant only if it is the creator of master. If file_priv->is_master is false, then a call to drm_setmaster_ioctl will invoke drm_new_set_master, which then allocates a new master for file_priv, and puts the old master.
This could be an issue in _drm_lease_held_master, because we dereference master to get master->dev, master->lessor, and master->leases.
With the same reasoning, in other parts of drm_lease.c, if there's an access to drm_file->master that's subsequently dereferenced, I added a lock around them.
I could definitely be mistaken on this, so apologies if this scenario doesn't arise.
You're right, I totally missed that setmaster can create a new master instance. And the kerneldoc for drm_file->master doesn't explain this and mention that we must hold drm_device.master_mutex while looking at that pointer. Can you pls do a patch which improves the documentation for that?
Now for the patch itself I'm not entirely sure what we should do. Leaking the dev->master_mutex into drm_lease.c just because of the setmaster ioctl is kinda unsightly. And we don't really care about the fpriv->master changing under us, we only need to make sure it doesn't get freed. And drm_master is refcounted already.
So alternative solution: We add a drm_file_get_master() function which calls drm_master_get under the lock, and we use that instead of directly derefencing drm_file->master? Ofc then needs drm_master_put instead of mutex_unlock. Kerneldoc should then also point at this new function as the correct way to look at drm_file->master state.
This way it's 100% clear we're dealing with a lifetime issue and not a consistency issues.
What do you think? -Daniel
Best wishes, Desmond
On 18/6/21 5:12 pm, Daniel Vetter wrote:
On Fri, Jun 18, 2021 at 5:05 AM Desmond Cheong Zhi Xi desmondcheongzx@gmail.com wrote:
On 18/6/21 1:12 am, Daniel Vetter wrote:
On Tue, Jun 15, 2021 at 10:36:45AM +0800, Desmond Cheong Zhi Xi wrote:
This patch ensures that the device's master mutex is acquired before accessing pointers to struct drm_master that are subsequently dereferenced. Without the mutex, the struct drm_master may be freed concurrently by another process calling drm_setmaster_ioctl(). This could then lead to use-after-free errors.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index da4f085fc09e..3e6f689236e5 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -107,10 +107,16 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) {
- bool ret;
if (!file_priv || !file_priv->master) return true;
- return _drm_lease_held_master(file_priv->master, id);
- mutex_lock(&file_priv->master->dev->master_mutex);
So maybe we have a bug somewhere, and the kerneldoc isn't 100% clear, but I thought file_priv->master is invariant over the lifetime of file_priv. So we don't need a lock to check anything here.
It's the drm_device->master derefence that gets us into trouble. Well also file_priv->is_owner is protected by dev->master_mutex.
So I think with your previous patch all the access here in drm_lease.c is ok and already protected? Or am I missing something?
Thanks, Daniel
My thinking was that file_priv->master is invariant only if it is the creator of master. If file_priv->is_master is false, then a call to drm_setmaster_ioctl will invoke drm_new_set_master, which then allocates a new master for file_priv, and puts the old master.
This could be an issue in _drm_lease_held_master, because we dereference master to get master->dev, master->lessor, and master->leases.
With the same reasoning, in other parts of drm_lease.c, if there's an access to drm_file->master that's subsequently dereferenced, I added a lock around them.
I could definitely be mistaken on this, so apologies if this scenario doesn't arise.
You're right, I totally missed that setmaster can create a new master instance. And the kerneldoc for drm_file->master doesn't explain this and mention that we must hold drm_device.master_mutex while looking at that pointer. Can you pls do a patch which improves the documentation for that?
Sounds good, I'll add it to the patch series.
Now for the patch itself I'm not entirely sure what we should do. Leaking the dev->master_mutex into drm_lease.c just because of the setmaster ioctl is kinda unsightly. And we don't really care about the fpriv->master changing under us, we only need to make sure it doesn't get freed. And drm_master is refcounted already.
So alternative solution: We add a drm_file_get_master() function which calls drm_master_get under the lock, and we use that instead of directly derefencing drm_file->master? Ofc then needs drm_master_put instead of mutex_unlock. Kerneldoc should then also point at this new function as the correct way to look at drm_file->master state.
This way it's 100% clear we're dealing with a lifetime issue and not a consistency issues.
What do you think? -Daniel
Makes sense to me, since the drm master itself holds the lease, as long as it isn't freed while we're using it, there's no need to prevent the value of fpriv->master from changing after we access it in drm_lease.c.
I was going to say that it may be unclear when to use the
master = drm_file_get_master(file_priv); ... drm_master_put(&master);
pattern, versus when to use
mutex_lock(&dev->master_mutex); master = file_priv->master; ... mutex_unlock(&dev->master_mutex);
. The second pattern, for example, is used in drm_getunique, and also in drm_setversion which calls drm_set_busid.
But on closer inspection, it's clearer to me now that those functions need the master_mutex because they access protected fields such as unique and unique_len.
Would it then be correct to state in the kerneldoc that drm_file_get_master() should be used to look at drm_file->master only if we aren't already holding master_mutex + have no other need to grab master_mutex?
On Fri, Jun 18, 2021 at 6:54 PM Desmond Cheong Zhi Xi desmondcheongzx@gmail.com wrote:
On 18/6/21 5:12 pm, Daniel Vetter wrote:
On Fri, Jun 18, 2021 at 5:05 AM Desmond Cheong Zhi Xi desmondcheongzx@gmail.com wrote:
On 18/6/21 1:12 am, Daniel Vetter wrote:
On Tue, Jun 15, 2021 at 10:36:45AM +0800, Desmond Cheong Zhi Xi wrote:
This patch ensures that the device's master mutex is acquired before accessing pointers to struct drm_master that are subsequently dereferenced. Without the mutex, the struct drm_master may be freed concurrently by another process calling drm_setmaster_ioctl(). This could then lead to use-after-free errors.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index da4f085fc09e..3e6f689236e5 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -107,10 +107,16 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) {
- bool ret;
if (!file_priv || !file_priv->master) return true;
- return _drm_lease_held_master(file_priv->master, id);
- mutex_lock(&file_priv->master->dev->master_mutex);
So maybe we have a bug somewhere, and the kerneldoc isn't 100% clear, but I thought file_priv->master is invariant over the lifetime of file_priv. So we don't need a lock to check anything here.
It's the drm_device->master derefence that gets us into trouble. Well also file_priv->is_owner is protected by dev->master_mutex.
So I think with your previous patch all the access here in drm_lease.c is ok and already protected? Or am I missing something?
Thanks, Daniel
My thinking was that file_priv->master is invariant only if it is the creator of master. If file_priv->is_master is false, then a call to drm_setmaster_ioctl will invoke drm_new_set_master, which then allocates a new master for file_priv, and puts the old master.
This could be an issue in _drm_lease_held_master, because we dereference master to get master->dev, master->lessor, and master->leases.
With the same reasoning, in other parts of drm_lease.c, if there's an access to drm_file->master that's subsequently dereferenced, I added a lock around them.
I could definitely be mistaken on this, so apologies if this scenario doesn't arise.
You're right, I totally missed that setmaster can create a new master instance. And the kerneldoc for drm_file->master doesn't explain this and mention that we must hold drm_device.master_mutex while looking at that pointer. Can you pls do a patch which improves the documentation for that?
Sounds good, I'll add it to the patch series.
Now for the patch itself I'm not entirely sure what we should do. Leaking the dev->master_mutex into drm_lease.c just because of the setmaster ioctl is kinda unsightly. And we don't really care about the fpriv->master changing under us, we only need to make sure it doesn't get freed. And drm_master is refcounted already.
So alternative solution: We add a drm_file_get_master() function which calls drm_master_get under the lock, and we use that instead of directly derefencing drm_file->master? Ofc then needs drm_master_put instead of mutex_unlock. Kerneldoc should then also point at this new function as the correct way to look at drm_file->master state.
This way it's 100% clear we're dealing with a lifetime issue and not a consistency issues.
What do you think? -Daniel
Makes sense to me, since the drm master itself holds the lease, as long as it isn't freed while we're using it, there's no need to prevent the value of fpriv->master from changing after we access it in drm_lease.c.
I was going to say that it may be unclear when to use the
master = drm_file_get_master(file_priv); ... drm_master_put(&master);pattern, versus when to use
mutex_lock(&dev->master_mutex); master = file_priv->master; ... mutex_unlock(&dev->master_mutex);. The second pattern, for example, is used in drm_getunique, and also in drm_setversion which calls drm_set_busid.
But on closer inspection, it's clearer to me now that those functions need the master_mutex because they access protected fields such as unique and unique_len.
Would it then be correct to state in the kerneldoc that drm_file_get_master() should be used to look at drm_file->master only if we aren't already holding master_mutex + have no other need to grab master_mutex?
Yeah that's sounds like a good decider for which variant to pick. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Desmond Cheong Zhi Xi