[PATCH 1/2] drm: Make control nodes master-less

[David Herrmann]

Hi

>>> You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
>>> I guess this, and Daniel's reply prompts a discussion about control
>>> nodes and the master concept.
>>>
>>> First I'd like to give some background about the use-case: I'd like to
>>> use the control node for a daemon that tells the vmwgfx driver about the
>>> host GUI layout of the screen: What connectors are enabled, the
>>> preferred mode of each output and some driver private information. The
>>> daemon will run as root and only a single instance per device. Trying to
>>> do this as-is will cause the vmwgfx driver to BUG() because it currently
>>> assumes one active master per device. Not one active master per minor
>>> (although that could be changed if needed).
>> Why do you tie this to DRM-Master? Can't you just limit these special
>> ioctls to the control-node and drop any master-requirements? This way
>> you don't care whether the FD on the control-node is master or not,
>> you just assume that access-rights on the control-node are highly
>> restricted so anyone opening it is privileged to issue these ioctls.
>
>
> That's exactly what I'm planning to do.
> The BUG() in vmwgfx that happens when a device gets two masters (one
> from legacy and one from control) has been there
> for some time and is related to other functionality [1].

Great.

>> Anyhow, imho we should just try to remove any master-handling from any
>> core DRM code. Ideally, only drm_drv.c deals with DRM-Master when
>> checking for permissions. Everything else should never attach any
>> information to these things. Especially the master-related driver
>> callbacks are undocumented and really weird. If we'd be able to drop
>> all this, I think we can start looking forward.
>
> [1]
> I'm partly guilty of that. There is an, IMHO quite nasty, security
> problem with the current master concept:
> Imagine you have a master (say X server) with a number of authenticated
> clients.
> Then the master drops master privileges and another X server becomes master.
> Now the old master's authenticated clients may still access the new X
> server's (legacy) shared buffers, since drm doesn't
> differentiate which master a client is authenticated with.
>
> This is, as far as I can tell, even violating X server security policy.
>
> While render nodes will make future solutions work around this, they
> don't plug this security problem, and since nobody else
> really seems to care, we use the driver hooks to suspend authenticated
> clients when their master drops master privileges, just like DRI1 used
> to do - hence the TTM lock tied to a master.
>
> So IMHO once there is a core drm solution in place for this, we should
> probably be ready to drop the driver master_set and master_drop hooks.

What shared buffers are you talking about here? GEM objects are tied
to drm_file, so one authenticated client can never access bos of other
clients. There is one exception: FLINK. However, that one is _not_
allowed on render-nodes.

drm_framebuffer objects are somewhat broken, indeed. But we modified
drmModeGetFB() to _not_ return any handle if you're not master. So
clients cannot get access to the underlying buffer. Furthermore, we
plan to tie FBs to drm_file the same way we do that for gem handles.
That should fix this leak (we need to allow CAP_SYS_ADMIN to access
any FB, though.. backwards compat..).

That's of course only the theory. Hand-crafted exec-buffers can
obviously access other buffers of other clients if you have no
stream-validation and/or virtual memory on the GPU. Imho, that's a
totally different issue, though.

Thanks
David