From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH 1/2] drm: Make control nodes master-less Date: Wed, 12 Mar 2014 14:36:04 +0100 Message-ID: <532062C4.8000704@vmware.com> References: <1392817242-15889-1-git-send-email-thellstrom@vmware.com> <531D8697.2090603@vmware.com> <53205416.1060503@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-2.vmware.com (smtp-outbound-2.vmware.com [208.91.2.13]) by gabe.freedesktop.org (Postfix) with ESMTP id 4B630FB032 for ; Wed, 12 Mar 2014 06:36:08 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: David Herrmann Cc: Dave Airlie , linux-graphics-maintainer@vmware.com, "dri-devel@lists.freedesktop.org" , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org 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