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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
