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

[Thomas Hellstrom <thellstrom@vmware.com>]

Hi,

On 03/12/2014 02:16 PM, David Herrmann wrote:
> 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.

Yes, I was referring to FLINK and its TTM counterpart.

But, as said, render-nodes provide a way to work around this issue for
future
display servers but I'm arguing you can't fix a security leak in an old
interface
by providing a new interface as long as the old interface remains.

>
> 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 sounds good.

>
> 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.
Agreed, although I think for "non-render-node (legacy) mode", suspending
authenticated clients on first-access-after-master-drop should fix that
as well.


>
> Thanks
> David

Thanks,
Thomas