[libvirt] [PATCH] security: dac: relabel spice rendernode

Cole Robinson posted 1 patch 6 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/c1e6e90d1c15dec8abfadec94b21c456f5df8dc5.1500308949.git.crobinso@redhat.com
src/security/security_dac.c | 61 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
[libvirt] [PATCH] security: dac: relabel spice rendernode
Posted by Cole Robinson 6 years, 9 months ago
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(+)

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;
-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: dac: relabel spice rendernode
Posted by Daniel P. Berrange 6 years, 9 months ago
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
Re: [libvirt] [PATCH] security: dac: relabel spice rendernode
Posted by Cole Robinson 6 years, 9 months ago
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
Re: [libvirt] [PATCH] security: dac: relabel spice rendernode
Posted by Daniel P. Berrange 6 years, 9 months ago
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