security: dac: relabel spice rendernode

Message ID c1e6e90d1c15dec8abfadec94b21c456f5df8dc5.1500308949.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson July 17, 2017, 4:31 p.m.
For a logged in user this a path like /dev/dri/renderD128 will have
default ownership root:video which won't work for the qemu:qemu user,
so we need to chown it.

Thankfully with the namespace work we don't need to worry about this
shutting out other legitimate users

https://bugzilla.redhat.com/show_bug.cgi?id=1460804
Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
Sidenote: Not sure about security_selinux changes... Fedora selinux policy
doesn't require relabeling /dev/dri/* nowadays so it isn't required to get
qemu to startup, and infact will probably cause issues for qemu:///session
and non-namespace qemu:///system

 src/security/security_dac.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Daniel P. Berrange July 17, 2017, 4:35 p.m. | #1
On Mon, Jul 17, 2017 at 12:31:50PM -0400, Cole Robinson wrote:
> For a logged in user this a path like /dev/dri/renderD128 will have

> default ownership root:video which won't work for the qemu:qemu user,

> so we need to chown it.

> 

> Thankfully with the namespace work we don't need to worry about this

> shutting out other legitimate users


We support turning off namespaces, in which case this will harm other
users. So at very least we need to make this conditional on namespaces
being enabled.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson July 17, 2017, 4:42 p.m. | #2
On 07/17/2017 12:35 PM, Daniel P. Berrange wrote:
> On Mon, Jul 17, 2017 at 12:31:50PM -0400, Cole Robinson wrote:

>> For a logged in user this a path like /dev/dri/renderD128 will have

>> default ownership root:video which won't work for the qemu:qemu user,

>> so we need to chown it.

>>

>> Thankfully with the namespace work we don't need to worry about this

>> shutting out other legitimate users

> 

> We support turning off namespaces, in which case this will harm other

> users. So at very least we need to make this conditional on namespaces

> being enabled.

> 


I can look into that, but then again it's basically the way the DAC driver
already works for potentially more invasive scenarios like /dev/sd*,
/dev/cdrom, USB devices etc etc

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrange July 18, 2017, 7:20 a.m. | #3
On Mon, Jul 17, 2017 at 12:42:12PM -0400, Cole Robinson wrote:
> On 07/17/2017 12:35 PM, Daniel P. Berrange wrote:

> > On Mon, Jul 17, 2017 at 12:31:50PM -0400, Cole Robinson wrote:

> >> For a logged in user this a path like /dev/dri/renderD128 will have

> >> default ownership root:video which won't work for the qemu:qemu user,

> >> so we need to chown it.

> >>

> >> Thankfully with the namespace work we don't need to worry about this

> >> shutting out other legitimate users

> > 

> > We support turning off namespaces, in which case this will harm other

> > users. So at very least we need to make this conditional on namespaces

> > being enabled.

> > 

> 

> I can look into that, but then again it's basically the way the DAC driver

> already works for potentially more invasive scenarios like /dev/sd*,

> /dev/cdrom, USB devices etc etc


My concern is that changing this will break apps in the active desktop
session that are currently using the graphics card too.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index ca7a6af6d..4c86e5fe8 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1371,6 +1371,57 @@  virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
 
 
 static int
+virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr,
+                               virDomainDefPtr def,
+                               virDomainGraphicsDefPtr gfx)
+
+{
+    virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    virSecurityLabelDefPtr seclabel;
+    uid_t user;
+    gid_t group;
+
+    seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
+    if (seclabel && !seclabel->relabel)
+        return 0;
+
+    if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0)
+        return -1;
+
+    if (gfx->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+        gfx->data.spice.gl == VIR_TRISTATE_BOOL_YES &&
+        gfx->data.spice.rendernode) {
+        if (virSecurityDACSetOwnership(priv, NULL,
+                                       gfx->data.spice.rendernode,
+                                       user, group) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
+
+static int
+virSecurityDACRestoreGraphicsLabel(virSecurityManagerPtr mgr,
+                               virDomainDefPtr def ATTRIBUTE_UNUSED,
+                               virDomainGraphicsDefPtr gfx)
+
+{
+    virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+
+    if (gfx->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+        gfx->data.spice.gl == VIR_TRISTATE_BOOL_YES &&
+        gfx->data.spice.rendernode) {
+        if (virSecurityDACRestoreFileLabel(priv,
+                                           gfx->data.spice.rendernode) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
+
+static int
 virSecurityDACSetInputLabel(virSecurityManagerPtr mgr,
                             virDomainDefPtr def,
                             virDomainInputDefPtr input)
@@ -1481,6 +1532,11 @@  virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
             rc = -1;
     }
 
+    for (i = 0; i < def->ngraphics; i++) {
+        if (virSecurityDACRestoreGraphicsLabel(mgr, def, def->graphics[i]) < 0)
+            return -1;
+    }
+
     for (i = 0; i < def->ninputs; i++) {
         if (virSecurityDACRestoreInputLabel(mgr, def, def->inputs[i]) < 0)
             rc = -1;
@@ -1601,6 +1657,11 @@  virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
             return -1;
     }
 
+    for (i = 0; i < def->ngraphics; i++) {
+        if (virSecurityDACSetGraphicsLabel(mgr, def, def->graphics[i]) < 0)
+            return -1;
+    }
+
     for (i = 0; i < def->ninputs; i++) {
         if (virSecurityDACSetInputLabel(mgr, def, def->inputs[i]) < 0)
             return -1;