[PATCH] drm: rework SET_MASTER and DROP_MASTER perm handling

[Adam Jackson]

On Wed, 2020-02-19 at 13:27 +0000, Emil Velikov wrote:

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

I'm not super worried. Everything about VTs is a pile of DoS scenarios
that userspace has to dance to avoid. It sounds like this is only
introducing new DoS scenarios for cases that previously simply did not
work.

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

Nitpick: no comma here.

>  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;

I'd mentioned this on IRC, and it doesn't need to be changed with this
patch, but it would be cool if the "does the device already have a
master" check just below here would return -EBUSY instead of -EINVAL so
userspace diagnostics have a chance of saying something useful. A quick
audit of the X drivers and weston shows no cases where we care about
the generated errno value beyond feeding it into strerror() so that
should also be safe.

Looks good otherwise, and these are definitely more reasonable
semantics. Thanks for taking this on!

Reviewed-by: Adam Jackson <ajax at redhat.com>

- ajax