Commit 56f0729a authored by Desmond Cheong Zhi Xi's avatar Desmond Cheong Zhi Xi Committed by Daniel Vetter
Browse files

drm: protect drm_master pointers in drm_lease.c 



drm_file->master pointers should be protected by
drm_device.master_mutex or drm_file.master_lookup_lock when being
dereferenced.

However, in drm_lease.c, there are multiple instances where
drm_file->master is accessed and dereferenced while neither lock is
held. This makes drm_lease.c vulnerable to use-after-free bugs.

We address this issue in 2 ways:

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

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

Reported-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: default avatarDesmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Reviewed-by: default avatarEmil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210712043508.11584-6-desmondcheongzx@gmail.com
parent 0b0860a3

drivers/gpu/drm/drm_auth.c

+25 −0
Original line number Diff line number Diff line
@@ -389,6 +389,31 @@ struct drm_master *drm_master_get(struct drm_master *master) 
}

EXPORT_SYMBOL(drm_master_get);



/**

 * drm_file_get_master - reference &drm_file.master of @file_priv

 * @file_priv: DRM file private

 *

 * Increments the reference count of @file_priv's &drm_file.master and returns

 * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL.

 *

 * Master pointers returned from this function should be unreferenced using

 * drm_master_put().

 */

struct drm_master *drm_file_get_master(struct drm_file *file_priv)

{

	struct drm_master *master = NULL;



	spin_lock(&file_priv->master_lookup_lock);

	if (!file_priv->master)

		goto unlock;

	master = drm_master_get(file_priv->master);



unlock:

	spin_unlock(&file_priv->master_lookup_lock);

	return master;

}

EXPORT_SYMBOL(drm_file_get_master);



static void drm_master_destroy(struct kref *kref)

{

	struct drm_master *master = container_of(kref, struct drm_master, refcount);

drivers/gpu/drm/drm_lease.c

+61 −20
Original line number Diff line number Diff line
@@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id) 
 */

bool _drm_lease_held(struct drm_file *file_priv, int id)

{

	if (!file_priv || !file_priv->master)

	bool ret;

	struct drm_master *master;



	if (!file_priv)

		return true;



	master = drm_file_get_master(file_priv);

	if (!master)

		return true;

	ret = _drm_lease_held_master(master, id);

	drm_master_put(&master);



	return _drm_lease_held_master(file_priv->master, id);

	return ret;

}



/**
@@ -128,13 +137,22 @@ bool drm_lease_held(struct drm_file *file_priv, int id) 
	struct drm_master *master;

	bool ret;



	if (!file_priv || !file_priv->master || !file_priv->master->lessor)

	if (!file_priv)

		return true;



	master = file_priv->master;

	master = drm_file_get_master(file_priv);

	if (!master)

		return true;

	if (!master->lessor) {

		ret = true;

		goto out;

	}

	mutex_lock(&master->dev->mode_config.idr_mutex);

	ret = _drm_lease_held_master(master, id);

	mutex_unlock(&master->dev->mode_config.idr_mutex);



out:

	drm_master_put(&master);

	return ret;

}
@@ -154,10 +172,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) 
	int count_in, count_out;

	uint32_t crtcs_out = 0;



	if (!file_priv || !file_priv->master || !file_priv->master->lessor)

	if (!file_priv)

		return crtcs_in;



	master = file_priv->master;

	master = drm_file_get_master(file_priv);

	if (!master)

		return crtcs_in;

	if (!master->lessor) {

		crtcs_out = crtcs_in;

		goto out;

	}

	dev = master->dev;



	count_in = count_out = 0;
@@ -176,6 +200,9 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) 
		count_in++;

	}

	mutex_unlock(&master->dev->mode_config.idr_mutex);



out:

	drm_master_put(&master);

	return crtcs_out;

}
@@ -489,7 +516,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, 
	size_t object_count;

	int ret = 0;

	struct idr leases;

	struct drm_master *lessor = lessor_priv->master;

	struct drm_master *lessor;

	struct drm_master *lessee = NULL;

	struct file *lessee_file = NULL;

	struct file *lessor_file = lessor_priv->filp;
@@ -501,12 +528,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, 
	if (!drm_core_check_feature(dev, DRIVER_MODESET))

		return -EOPNOTSUPP;



	/* Do not allow sub-leases */

	if (lessor->lessor) {

		DRM_DEBUG_LEASE("recursive leasing not allowed\n");

		return -EINVAL;

	}



	/* need some objects */

	if (cl->object_count == 0) {

		DRM_DEBUG_LEASE("no objects in lease\n");
@@ -518,12 +539,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, 
		return -EINVAL;

	}



	lessor = drm_file_get_master(lessor_priv);

	/* Do not allow sub-leases */

	if (lessor->lessor) {

		DRM_DEBUG_LEASE("recursive leasing not allowed\n");

		ret = -EINVAL;

		goto out_lessor;

	}



	object_count = cl->object_count;



	object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),

			array_size(object_count, sizeof(__u32)));

	if (IS_ERR(object_ids))

		return PTR_ERR(object_ids);

	if (IS_ERR(object_ids)) {

		ret = PTR_ERR(object_ids);

		goto out_lessor;

	}



	idr_init(&leases);
@@ -534,14 +565,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, 
	if (ret) {

		DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);

		idr_destroy(&leases);

		return ret;

		goto out_lessor;

	}



	/* Allocate a file descriptor for the lease */

	fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));

	if (fd < 0) {

		idr_destroy(&leases);

		return fd;

		ret = fd;

		goto out_lessor;

	}



	DRM_DEBUG_LEASE("Creating lease\n");
@@ -577,6 +609,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, 
	/* Hook up the fd */

	fd_install(fd, lessee_file);



	drm_master_put(&lessor);

	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");

	return 0;
@@ -586,6 +619,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, 
out_leases:

	put_unused_fd(fd);



out_lessor:

	drm_master_put(&lessor);

	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);

	return ret;

}
@@ -608,7 +643,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, 
	struct drm_mode_list_lessees *arg = data;

	__u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr);

	__u32 count_lessees = arg->count_lessees;

	struct drm_master *lessor = lessor_priv->master, *lessee;

	struct drm_master *lessor, *lessee;

	int count;

	int ret = 0;
@@ -619,6 +654,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, 
	if (!drm_core_check_feature(dev, DRIVER_MODESET))

		return -EOPNOTSUPP;



	lessor = drm_file_get_master(lessor_priv);

	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);



	mutex_lock(&dev->mode_config.idr_mutex);
@@ -642,6 +678,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, 
		arg->count_lessees = count;



	mutex_unlock(&dev->mode_config.idr_mutex);

	drm_master_put(&lessor);



	return ret;

}
@@ -661,7 +698,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, 
	struct drm_mode_get_lease *arg = data;

	__u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr);

	__u32 count_objects = arg->count_objects;

	struct drm_master *lessee = lessee_priv->master;

	struct drm_master *lessee;

	struct idr *object_idr;

	int count;

	void *entry;
@@ -675,6 +712,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, 
	if (!drm_core_check_feature(dev, DRIVER_MODESET))

		return -EOPNOTSUPP;



	lessee = drm_file_get_master(lessee_priv);

	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);



	mutex_lock(&dev->mode_config.idr_mutex);
@@ -702,6 +740,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, 
		arg->count_objects = count;



	mutex_unlock(&dev->mode_config.idr_mutex);

	drm_master_put(&lessee);



	return ret;

}
@@ -720,7 +759,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, 
				void *data, struct drm_file *lessor_priv)

{

	struct drm_mode_revoke_lease *arg = data;

	struct drm_master *lessor = lessor_priv->master;

	struct drm_master *lessor;

	struct drm_master *lessee;

	int ret = 0;
@@ -730,6 +769,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, 
	if (!drm_core_check_feature(dev, DRIVER_MODESET))

		return -EOPNOTSUPP;



	lessor = drm_file_get_master(lessor_priv);

	mutex_lock(&dev->mode_config.idr_mutex);



	lessee = _drm_find_lessee(lessor, arg->lessee_id);
@@ -750,6 +790,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, 


fail:

	mutex_unlock(&dev->mode_config.idr_mutex);

	drm_master_put(&lessor);



	return ret;

}

include/drm/drm_auth.h

+1 −0
Original line number Diff line number Diff line
@@ -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);

include/drm/drm_file.h

+6 −0
Original line number Diff line number Diff line
@@ -233,6 +233,12 @@ struct drm_file { 
	 * this only matches &drm_device.master if the master is the currently

	 * active one.

	 *

	 * When dereferencing this pointer, either hold struct

	 * &drm_device.master_mutex for the duration of the pointer's use, or

	 * use drm_file_get_master() if struct &drm_device.master_mutex is not

	 * currently held and there is no other need to hold it. This prevents

	 * @master from being freed during use.

	 *

	 * See also @authentication and @is_master and the :ref:`section on

	 * primary nodes and authentication <drm_primary_node>`.

	 */