[PATCH] drm: rework SET_MASTER and DROP_MASTER perm handling

[Pekka Paalanen]

On Wed, 19 Feb 2020 13:27:28 +0000
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> From: Emil Velikov <emil.velikov at collabora.com>
> 

...

> +/*
> + * 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.
> + *

Hi,

sorry I had to trim this email harshly, but Google did not want to
deliver it otherwise.

I agree that being able to drop master without CAP_SYS_ADMIN sounds
like a good thing.

> + *
> + * 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:
> + *  - using it is not possible on some platforms
> + *  - 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.

I understand the drop master part, because it is needed to get rid of
apps that accidentally gain DRM master because they were the first one
to open the DRM device (on a particular VT?). It could happen e.g. if a
Wayland client is inspecting DRM devices to figure what it wants to
lease while the user has VT-switched to a text-mode VT, I guess. E.g.
starting a Wayland VR compositor from a VT for whatever reason.

The set master without CAP_SYS_ADMIN part I don't understand.

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

Why is non-root without any logind-equivalent a use case that should
work?

Why did DRM set/drop master use to require CAP_SYS_ADMIN in the first
place?

What else happens if we allow DRM set master more than just for
CAP_SYS_ADMIN?

Does this interact with DRM leasing?

Looks like drmIsMaster() should be unaffected at least:
https://patchwork.kernel.org/patch/10776525/

> + * - startx - some drivers work fine regardless
> + * - 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;

In case a helper e.g. logind opens the device, is file_priv->pid then
referring to logind regardless of what happens afterwards?

> +
> +	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;
>  
> @@ -229,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);

Why does drop-master need any kind of permission check? Why could it
not be free for all?


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200306/465b68b1/attachment.sig>