| Message ID | 1392817242-15889-1-git-send-email-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi! Could someone please review these two patches? Thanks, Thomas On 02/19/2014 02:40 PM, Thomas Hellstrom wrote: > Like for render-nodes, there is no point in maintaining the master concept > for control nodes, so set the struct drm_file::master pointer to NULL. > > At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always > allowed when called through the control node. Previously the caller also > needed to be master. > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > --- > drivers/gpu/drm/drm_drv.c | 5 +++-- > drivers/gpu/drm/drm_fops.c | 5 +++-- > include/drm/drmP.h | 5 +++++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 345be03..42af8bd 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp, > retcode = -EINVAL; > } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || > ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || > - ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || > - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || > + (((ioctl->flags & DRM_MASTER) && !file_priv->is_master) && > + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) || > + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) || > (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) { > retcode = -EACCES; > } else { > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index 7f2af9a..08a3196 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, > /* if there is no current master make this fd it, but do not create > * any master object for render clients */ > mutex_lock(&dev->struct_mutex); > - if (!priv->minor->master && !drm_is_render_client(priv)) { > + if (!priv->minor->master && !drm_is_render_client(priv) && > + !drm_is_control(priv)) { > /* create a new master */ > priv->minor->master = drm_master_create(priv->minor); > if (!priv->minor->master) { > @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, > goto out_close; > } > } > - } else if (!drm_is_render_client(priv)) { > + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) { > /* get a reference to the master */ > priv->master = drm_master_get(priv->minor->master); > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 04a7f31..ff68e26 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_RENDER; > } > > +static inline bool drm_is_control(struct drm_file *file_priv) > +{ > + return file_priv->minor->type == DRM_MINOR_CONTROL; > +} > + > /******************************************************************/ > /** \name Internal function definitions */ > /*@{*/
On Wed, Feb 19, 2014 at 02:40:41PM +0100, Thomas Hellstrom wrote: > Like for render-nodes, there is no point in maintaining the master concept > for control nodes, so set the struct drm_file::master pointer to NULL. > > At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always > allowed when called through the control node. Previously the caller also > needed to be master. > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > --- > drivers/gpu/drm/drm_drv.c | 5 +++-- > drivers/gpu/drm/drm_fops.c | 5 +++-- > include/drm/drmP.h | 5 +++++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 345be03..42af8bd 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp, > retcode = -EINVAL; > } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || > ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || > - ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || > - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || > + (((ioctl->flags & DRM_MASTER) && !file_priv->is_master) && > + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) || > + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) || > (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) { This is hideous to review ;-) I think it would be really good to extract this entire condition into a drm_check_ioctl_acces(ioctl, file_priv) helper and untangle all the different cases a bit by splitting it up into if checks with individual return false/true; statements. With that bit of polish for the next reviewer's sanity applied both patches are Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> I guess with this change we could move the master pointer from drm_minor to drm_device, which would make it really clear that there's only ever one master per device. But that's one giant sed job, so meh ;-) One thing I'm unsure about is whether we want/need to have the master concept on the control node, too. logind uses set/dropmaster as a kms-specific revoke support, so if we ever want to switch to using control nodes for display servers we'd need to shuffle this a bit again. Otoh no one is using control nodes for real afaik, so we likely need some interface polishing anyway. And for non-root display servers we can block out all the awful legacy drm ioctls easily, so just keeping on using legacy nodes isn't a security issue for that use-case. Cheers, Daniel > retcode = -EACCES; > } else { > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index 7f2af9a..08a3196 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, > /* if there is no current master make this fd it, but do not create > * any master object for render clients */ > mutex_lock(&dev->struct_mutex); > - if (!priv->minor->master && !drm_is_render_client(priv)) { > + if (!priv->minor->master && !drm_is_render_client(priv) && > + !drm_is_control(priv)) { > /* create a new master */ > priv->minor->master = drm_master_create(priv->minor); > if (!priv->minor->master) { > @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, > goto out_close; > } > } > - } else if (!drm_is_render_client(priv)) { > + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) { > /* get a reference to the master */ > priv->master = drm_master_get(priv->minor->master); > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 04a7f31..ff68e26 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_RENDER; > } > > +static inline bool drm_is_control(struct drm_file *file_priv) > +{ > + return file_priv->minor->type == DRM_MINOR_CONTROL; > +} > + > /******************************************************************/ > /** \name Internal function definitions */ > /*@{*/ > -- > 1.7.10.4 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi On Wed, Feb 19, 2014 at 2:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: > Like for render-nodes, there is no point in maintaining the master concept > for control nodes, so set the struct drm_file::master pointer to NULL. > > At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always > allowed when called through the control node. Previously the caller also > needed to be master. > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > --- > drivers/gpu/drm/drm_drv.c | 5 +++-- > drivers/gpu/drm/drm_fops.c | 5 +++-- > include/drm/drmP.h | 5 +++++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 345be03..42af8bd 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp, > retcode = -EINVAL; > } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || > ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || > - ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || > - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || > + (((ioctl->flags & DRM_MASTER) && !file_priv->is_master) && > + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) || > + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) || > (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) { > retcode = -EACCES; > } else { > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index 7f2af9a..08a3196 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, > /* if there is no current master make this fd it, but do not create > * any master object for render clients */ > mutex_lock(&dev->struct_mutex); > - if (!priv->minor->master && !drm_is_render_client(priv)) { > + if (!priv->minor->master && !drm_is_render_client(priv) && > + !drm_is_control(priv)) { > /* create a new master */ > priv->minor->master = drm_master_create(priv->minor); > if (!priv->minor->master) { > @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, > goto out_close; > } > } > - } else if (!drm_is_render_client(priv)) { > + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) { > /* get a reference to the master */ > priv->master = drm_master_get(priv->minor->master); Maybe I missed some other patches, but how is this going to work? drm_crtc.c expects priv->master to be non-NULL (eg., drm_mode_getresources()). This is fine for render-nodes as they don't allow any of those ioctls, but the control-node does allow them. Thanks David > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 04a7f31..ff68e26 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_RENDER; > } > > +static inline bool drm_is_control(struct drm_file *file_priv) > +{ > + return file_priv->minor->type == DRM_MINOR_CONTROL; > +} > + > /******************************************************************/ > /** \name Internal function definitions */ > /*@{*/ > -- > 1.7.10.4 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 03/05/2014 03:32 PM, David Herrmann wrote: > Hi > > On Wed, Feb 19, 2014 at 2:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote: >> Like for render-nodes, there is no point in maintaining the master concept >> for control nodes, so set the struct drm_file::master pointer to NULL. >> >> At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always >> allowed when called through the control node. Previously the caller also >> needed to be master. >> >> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> >> --- >> drivers/gpu/drm/drm_drv.c | 5 +++-- >> drivers/gpu/drm/drm_fops.c | 5 +++-- >> include/drm/drmP.h | 5 +++++ >> 3 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 345be03..42af8bd 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp, >> retcode = -EINVAL; >> } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || >> ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || >> - ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || >> - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || >> + (((ioctl->flags & DRM_MASTER) && !file_priv->is_master) && >> + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) || >> + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) || >> (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) { >> retcode = -EACCES; >> } else { >> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c >> index 7f2af9a..08a3196 100644 >> --- a/drivers/gpu/drm/drm_fops.c >> +++ b/drivers/gpu/drm/drm_fops.c >> @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, >> /* if there is no current master make this fd it, but do not create >> * any master object for render clients */ >> mutex_lock(&dev->struct_mutex); >> - if (!priv->minor->master && !drm_is_render_client(priv)) { >> + if (!priv->minor->master && !drm_is_render_client(priv) && >> + !drm_is_control(priv)) { >> /* create a new master */ >> priv->minor->master = drm_master_create(priv->minor); >> if (!priv->minor->master) { >> @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, >> goto out_close; >> } >> } >> - } else if (!drm_is_render_client(priv)) { >> + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) { >> /* get a reference to the master */ >> priv->master = drm_master_get(priv->minor->master); > Maybe I missed some other patches, but how is this going to work? > drm_crtc.c expects priv->master to be non-NULL (eg., > drm_mode_getresources()). This is fine for render-nodes as they don't > allow any of those ioctls, but the control-node does allow them. > > Thanks > David > > 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). Looking at http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/ it doesn't really make sense to keep the master concept on the control node, but it perhaps makes sense to keep it on future modesetting nodes to allow multiple modesetters to open the same device node? In that case all master access in drm_crtc.c would be changed for now to be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY /Thomas
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. > Looking at > > http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/ > > it doesn't really make sense to keep the master concept on the control > node, but it perhaps makes sense to keep it on future modesetting nodes > to allow multiple modesetters to open the same device node? > > In that case all master access in drm_crtc.c would be changed for now to > be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY The Master-concept only really makes sense for operations affecting global state. On non-legacy drivers this is basically KMS. Therefore, I see no reason to extend the master-concept to non-modeset/primary nodes. So ACK on changing drm_crtc.c to check for master only if minor==DRM_MINOR_PRIMARY/LEGACY (there are pending patches renaming it..). The problem with the modeset-node concept described in my blog-post is KMS-object hotplugging. We don't support it.. and I don't see any proper way to extend our API in a suitable way. The only thing I still have planned is to allow creating multiple DRM_MINOR_PRIMARY/LEGACY nodes and splitting mode-objects among them. We would have to do that before anyone uses the device to prevent objects from disappearing, but this could easily be done as udev-action. Thus, setting up multiple KMS interfaces on a single card. 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. Thanks David
On 03/12/2014 10:45 AM, 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]. >> Looking at >> >> https://urldefense.proofpoint.com/v1/url?u=http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=V8cVVz4Hn%2FjsqGcRA9UoPCYNUTXvS%2BA21CW7xsg5Mwc%3D%0A&s=3b7c73b3db46fe8d9fdc5aa7e1f83939e84be61977db211c24b40ef81d911e28 >> >> it doesn't really make sense to keep the master concept on the control >> node, but it perhaps makes sense to keep it on future modesetting nodes >> to allow multiple modesetters to open the same device node? >> >> In that case all master access in drm_crtc.c would be changed for now to >> be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY > The Master-concept only really makes sense for operations affecting > global state. On non-legacy drivers this is basically KMS. Therefore, > I see no reason to extend the master-concept to non-modeset/primary > nodes. So ACK on changing drm_crtc.c to check for master only if > minor==DRM_MINOR_PRIMARY/LEGACY (there are pending patches renaming > it..). Great. > The problem with the modeset-node concept described in my blog-post is > KMS-object hotplugging. We don't support it.. and I don't see any > proper way to extend our API in a suitable way. The only thing I still > have planned is to allow creating multiple DRM_MINOR_PRIMARY/LEGACY > nodes and splitting mode-objects among them. We would have to do that > before anyone uses the device to prevent objects from disappearing, > but this could easily be done as udev-action. Thus, setting up > multiple KMS interfaces on a single card. OK. > > 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. /Thomas > > Thanks > David
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
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
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 345be03..42af8bd 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp, retcode = -EINVAL; } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) || - ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || + (((ioctl->flags & DRM_MASTER) && !file_priv->is_master) && + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) || + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) || (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) { retcode = -EACCES; } else { diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 7f2af9a..08a3196 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* if there is no current master make this fd it, but do not create * any master object for render clients */ mutex_lock(&dev->struct_mutex); - if (!priv->minor->master && !drm_is_render_client(priv)) { + if (!priv->minor->master && !drm_is_render_client(priv) && + !drm_is_control(priv)) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, goto out_close; } } - } else if (!drm_is_render_client(priv)) { + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 04a7f31..ff68e26 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_RENDER; } +static inline bool drm_is_control(struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_CONTROL; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/
Like for render-nodes, there is no point in maintaining the master concept for control nodes, so set the struct drm_file::master pointer to NULL. At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always allowed when called through the control node. Previously the caller also needed to be master. Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> --- drivers/gpu/drm/drm_drv.c | 5 +++-- drivers/gpu/drm/drm_fops.c | 5 +++-- include/drm/drmP.h | 5 +++++ 3 files changed, 11 insertions(+), 4 deletions(-)