On Mon, Feb 10, 2020 at 8:01 PM Emil Velikov emil.l.velikov@gmail.com wrote:
Thanks for having a look Daniel.
On Fri, 7 Feb 2020 at 13:29, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Feb 05, 2020 at 05:48:39PM +0000, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
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 allows for any application which cannot rely on systemd-logind being present (for whichever reason), to drop it's master capabilities (and regain them at later point) w/o being ran as root.
See the comment above drm_master_check_perm() for more details.
Cc: Adam Jackson ajax@redhat.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com
This effectively supersedes an earlier patch [1] incorporating ajax's feedback (from IRC ages ago).
[1] https://patchwork.freedesktop.org/patch/268977/
drivers/gpu/drm/drm_auth.c | 59 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_ioctl.c | 4 +-- include/drm/drm_file.h | 11 +++++++ 3 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index cc9acd986c68..01d9e35c0106 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -135,6 +135,7 @@ static int drm_set_master(struct drm_device *dev, struct drm_file *fpriv, } }
fpriv->was_master = (ret == 0); return ret;}
@@ -179,12 +180,64 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv) return ret; }
+/*
- In the olden days the SET/DROP_MASTER ioctls used to return EACCES when
- CAP_SYS_ADMIN was not set.
- Even though the first client is _always_ master, it also had to be run as
- root, otherwise SET/DROP_MASTER would fail. In those cases no other client
- could become master ... EVER.
- Resulting in 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) 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 obstacles:
- using it is not possible on some platforms, or
- 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 root permission or simply not calling DROP_MASTER.
- Here we implement the next best thing:
- We enforce the CAP_SYS_ADMIN check only if the client was not a master
- before. We distinguish between the original master client (say logind) and
- another client which has the fd passed (say Xorg) by comparing the pids.
- As a result this fixes, the following when using root-less build w/o logind
- startx - some drivers work fine regardless
- weston
- various compositors based on wlroots
- */
I think this breaks logind security. With logind no compositor can open the device node directly, hence no compositor can accidentally become the master and block everyone else.
I've explicitly considered this case. AFAICT this patch does not change any of the contract. If you think there's a scenario where things have broken, please let me know.
And for the vt switch logind is the only one that can grant master rights, and it can make sure that the right compositor gets them. And if the old compositor is non-cooperating, it can also forcefully remove master rights.
Yes logind does set/drop master on VT switch, session setup/teardown, etc.
To take this a step further, there is no logind API or dbus method for compositors to only set/drop master. Thus logind ensures that compositors are in sane state.
But with this here we lift this restriction if a compositor has ever been master. So the following thing could happen:
- We have 3 compositors for different users C1, C2, C3
- We switch from C1 to C2
- While we switch no one is master for a moment, which means C3 could sneak in and do a quick setmaster, and become master
- Everything would come crashing done since logind believes it already revoked master for C1, but somehow it now cant grant master to C2
Does this scenario consider that all three compositors are logind users? If so, C3 should not be able to set or drop master. Since it got its fd from logind:
- `file_priv->pid` will point to systemd-logind, and
- `task_pid(current)` will point to the respective compositor
-> EACCES will be returned to any compositor calling drmSetMaster.
Regardless of my patch, C3 can open() and simply not release the master. Assuming it's the first DRM client of course - say switch to VTx + login + start C3.
Hm ... I guess this works indeed. Or should. I'm mildly freaked out that we're checking for opener_pid == current->pid. Not sure how many other security assumptions we're breaking.
I've been lucky enough to spot various ways to softlock my system... even when the compositor is using logind ;-) If you're really interested I can share, but I'm worried that people may see them as bashing at logind.
I'm not sure we can even support these two models at the same time.
+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)Isn't this a typo? Why should we only allow this if the opener is someone else ... that looks like the logind approach? Or is my bolean logic parser broken again.
Thanks for spotting it. Indeed that should be:
if (file_priv->pid == task_pid(current) && file_priv->was_master) return 0;
Modulo any objections, I'll do proper testing and submit a non RFC version. The inline comments will explicitly mention your concerns and why the patch is safe.
Given the above bug I think a solid igt for both the logind and the non-logind scenario is needed. We have some helpers to drop root and fork stuff and all that, so shouldn't be many lines. -Daniel