Commit 45bc3d26 authored by Emil Velikov's avatar Emil Velikov Committed by Emil Velikov
Browse files

drm: rework SET_MASTER and DROP_MASTER perm handling 



This commit reworks the permission handling of the two ioctls. In
particular it enforced the CAP_SYS_ADMIN check only, if:
 - we're issuing the ioctl from process other than the one which opened
the node, and
 - we are, or were master in the past

This ensures that we:
 - do not regress the systemd-logind style of DRM_MASTER arbitrator
 - allow applications which do not use systemd-logind to drop their
master capabilities (and regain them at later point) ... w/o running as
root.

See the comment above drm_master_check_perm() for more details.

v1:
 - Tweak wording, fixup all checks, add igt test

v2:
 - Add a few more comments, grammar nitpicks.

Cc: Adam Jackson <ajax@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Testcase: igt/core_setmaster/master-drop-set-user
Signed-off-by: default avatarEmil Velikov <emil.velikov@collabora.com>
Reviewed-by: default avatarAdam Jackson <ajax@redhat.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200319172930.230583-1-emil.l.velikov@gmail.com
parent c7ccc1b7

drivers/gpu/drm/drm_auth.c

+67 −0
Original line number Diff line number Diff line
@@ -135,6 +135,7 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv, 
		}

	}



	fpriv->was_master = (ret == 0);

	return ret;

}
@@ -174,12 +175,72 @@ out_err: 
	return ret;

}



/*

 * In the olden days the SET/DROP_MASTER ioctls used to return EACCES when

 * CAP_SYS_ADMIN was not set. This was used to prevent rogue applications

 * from becoming master and/or failing to release it.

 *

 * At the same time, the first client (for a given VT) is _always_ master.

 * Thus in order for the ioctls to succeed, one had to _explicitly_ run the

 * application as root or flip the setuid bit.

 *

 * If the CAP_SYS_ADMIN was missing, no other client could become master...

 * EVER :-( Leading to a) the graphics session dying badly or b) a completely

 * locked session.

 *

 *

 * As some point systemd-logind was introduced to orchestrate and delegate

 * master as applicable. It does so by opening the fd and passing it to users

 * while in itself logind a) does the set/drop master per users' request and

 * b)  * implicitly drops master on VT switch.

 *

 * Even though logind looks like the future, there are a few issues:

 *  - some platforms don't have equivalent (Android, CrOS, some BSDs) so

 * root is required _solely_ for SET/DROP MASTER.

 *  - applications may not be updated to use it,

 *  - any client which fails to drop master* can DoS the application using

 * logind, to a varying degree.

 *

 * * Either due missing CAP_SYS_ADMIN or simply not calling DROP_MASTER.

 *

 *

 * Here we implement the next best thing:

 *  - ensure the logind style of fd passing works unchanged, and

 *  - allow a client to drop/set master, iff it is/was master at a given point

 * in time.

 *

 * Note: DROP_MASTER cannot be free for all, as an arbitrator user could:

 *  - DoS/crash the arbitrator - details would be implementation specific

 *  - open the node, become master implicitly and cause issues

 *

 * As a result this fixes the following when using root-less build w/o logind

 * - startx

 * - weston

 * - various compositors based on wlroots

 */

static int

drm_master_check_perm(struct drm_device *dev, struct drm_file *file_priv)

{

	if (file_priv->pid == task_pid(current) && file_priv->was_master)

		return 0;



	if (!capable(CAP_SYS_ADMIN))

		return -EACCES;



	return 0;

}



int drm_setmaster_ioctl(struct drm_device *dev, void *data,

			struct drm_file *file_priv)

{

	int ret = 0;



	mutex_lock(&dev->master_mutex);



	ret = drm_master_check_perm(dev, file_priv);

	if (ret)

		goto out_unlock;



	if (drm_is_current_master(file_priv))

		goto out_unlock;
@@ -224,6 +285,12 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, 
	int ret = -EINVAL;



	mutex_lock(&dev->master_mutex);



	ret = drm_master_check_perm(dev, file_priv);

	if (ret)

		goto out_unlock;



	ret = -EINVAL;

	if (!drm_is_current_master(file_priv))

		goto out_unlock;

drivers/gpu/drm/drm_ioctl.c

+2 −2
Original line number Diff line number Diff line
@@ -599,8 +599,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { 
	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_SET_SAREA_CTX, drm_legacy_setsareactx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),

	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH),



	DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, DRM_ROOT_ONLY),

	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_ROOT_ONLY),

	DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, 0),

	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, 0),



	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY),

	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),

include/drm/drm_file.h

+11 −0
Original line number Diff line number Diff line
@@ -201,6 +201,17 @@ struct drm_file { 
	 */

	bool writeback_connectors;



	/**

	 * @was_master:

	 *

	 * This client has or had, master capability. Protected by struct

	 * &drm_device.master_mutex.

	 *

	 * This is used to ensure that CAP_SYS_ADMIN is not enforced, if the

	 * client is or was master in the past.

	 */

	bool was_master;



	/**

	 * @is_master:

	 *

CRA Git | Maintained and supported by SUSTech CRA and CCSE