Re: [PATCH v2 2/2] selinux: add security selinux function to label tapfd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On 10/15/2012 08:34 PM, Martin Kletzander wrote:

On 10/15/2012 11:03 AM, Guannan Ren wrote:

BZ:https://bugzilla.redhat.com/show_bug.cgi?id=851981
When using macvtap, a character device gets first created by
kernel with name /dev/tapN, its selinux context is:
system_u:object_r:device_t:s0

Shortly, when udev gets notification when new file is created
in /dev, it will then jump in and relabel this file back to the
expected default context:
system_u:object_r:tun_tap_device_t:s0

There is a time gap happened.
Sometimes, it will have migration failed, AVC error message:
type=AVC msg=audit(1349858424.233:42507): avc:  denied  { read write } for
pid=19926 comm="qemu-kvm" path="/dev/tap33" dev=devtmpfs ino=131524
scontext=unconfined_u:system_r:svirt_t:s0:c598,c908
tcontext=system_u:object_r:device_t:s0 tclass=chr_file

This patch will label the tapfd device before qemu process starts:
system_u:object_r:tun_tap_device_t:MCS(MCS from seclabel->label)
---
  src/libvirt_private.syms         |   1 +
  src/qemu/qemu_command.c          |   4 ++
  src/security/security_apparmor.c |  10 ++++
  src/security/security_dac.c      |   9 +++
  src/security/security_driver.h   |   4 ++
  src/security/security_manager.c  |  11 ++++
  src/security/security_manager.h  |   3 +
  src/security/security_nop.c      |   3 +-
  src/security/security_selinux.c  | 118 +++++++++++++++++++++++++++++++--------
  src/security/security_stack.c    |  18 ++++++
  10 files changed, 156 insertions(+), 25 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6ea1308..1703f6d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1056,6 +1056,7 @@ virSecurityManagerSetHostdevLabel;
  virSecurityManagerSetProcessLabel;
  virSecurityManagerSetSavedStateLabel;
  virSecurityManagerSetSocketLabel;
+virSecurityManagerSetTapFDLabel;
  virSecurityManagerStackAddNested;
  virSecurityManagerVerify;
  virSecurityManagerGetMountOptions;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d590df6..239592c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5412,6 +5412,10 @@ qemuBuildCommandLine(virConnectPtr conn,
                      if (tapfd < 0)
                          goto error;
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager, 
+                                                    def, tapfd) < 0)
+                    goto error;
+
                      last_good_net = i;
                      virCommandTransferFD(cmd, tapfd);
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c 
index d3f9d9e..1315fe1 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -872,6 +872,15 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr,
      return reload_profile(mgr, def, fd_path, true);
  }
+/* TODO need code here */ 
+static int
+AppArmorSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+                      virDomainDefPtr def ATTRIBUTE_UNUSED,
+                      int fd ATTRIBUTE_UNUSED)
+{
+    return 0;
+}
+
  virSecurityDriver virAppArmorSecurityDriver = {
      .privateDataLen                     = 0,
      .name                               = SECURITY_APPARMOR_NAME,
@@ -908,4 +917,5 @@ virSecurityDriver virAppArmorSecurityDriver = {
      .domainRestoreSavedStateLabel       = AppArmorRestoreSavedStateLabel,
.domainSetSecurityImageFDLabel = AppArmorSetImageFDLabel, 
+    .domainSetSecurityTapFDLabel        = AppArmorSetTapFDLabel,
  };
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index f126aa5..9dbf95d 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1029,6 +1029,14 @@ virSecurityDACSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
      return 0;
  }
+static int 
+virSecurityDACSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+                            virDomainDefPtr def ATTRIBUTE_UNUSED,
+                            int fd ATTRIBUTE_UNUSED)
+{
+    return 0;
+}
+
  static char *virSecurityDACGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
                                             virDomainDefPtr vm ATTRIBUTE_UNUSED) {
      return NULL;
@@ -1070,6 +1078,7 @@ virSecurityDriver virSecurityDriverDAC = {
      .domainRestoreSavedStateLabel       = virSecurityDACRestoreSavedStateLabel,
.domainSetSecurityImageFDLabel = virSecurityDACSetImageFDLabel, 
+    .domainSetSecurityTapFDLabel        = virSecurityDACSetTapFDLabel,
.domainGetSecurityMountOptions = virSecurityDACGetMountOptions, 
  };
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 8f52ec5..d49b401 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -95,6 +95,9 @@ typedef int (*virSecurityDomainSecurityVerify) (virSecurityManagerPtr mgr,
  typedef int (*virSecurityDomainSetImageFDLabel) (virSecurityManagerPtr mgr,
                                                   virDomainDefPtr def,
                                                   int fd);
+typedef int (*virSecurityDomainSetTapFDLabel) (virSecurityManagerPtr mgr,
+                                               virDomainDefPtr def,
+                                               int fd);
  typedef char *(*virSecurityDomainGetMountOptions) (virSecurityManagerPtr mgr,
                                                           virDomainDefPtr def);
@@ -134,6 +137,7 @@ struct _virSecurityDriver { 
      virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel;
virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel; 
+    virSecurityDomainSetTapFDLabel domainSetSecurityTapFDLabel;
virSecurityDomainGetMountOptions domainGetSecurityMountOptions; 
  };
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 40c8b7e..d446607 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -469,6 +469,17 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr,
      return -1;
  }
+int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, 
+                                    virDomainDefPtr vm,
+                                    int fd)
+{
+    if (mgr->drv->domainSetSecurityTapFDLabel)
+        return mgr->drv->domainSetSecurityTapFDLabel(mgr, vm, fd);
+
+    virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
+    return -1;
+}
+
  char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr,
                                          virDomainDefPtr vm)
  {
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index b3bc191..1fdaf8e 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -105,6 +105,9 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr,
  int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr,
                                        virDomainDefPtr def,
                                        int fd);
+int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr,
+                                    virDomainDefPtr vm,
+                                    int fd);
  char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr,
                                                virDomainDefPtr vm);
  virSecurityManagerPtr*
diff --git a/src/security/security_nop.c b/src/security/security_nop.c
index b56971c..86f644b 100644
--- a/src/security/security_nop.c
+++ b/src/security/security_nop.c
@@ -204,7 +204,8 @@ virSecurityDriver virSecurityDriverNop = {
      .domainSetSavedStateLabel           = virSecurityDomainSetSavedStateLabelNop,
      .domainRestoreSavedStateLabel       = virSecurityDomainRestoreSavedStateLabelNop,
- .domainSetSecurityImageFDLabel = virSecurityDomainSetFDLabelNop, 
+    .domainSetSecurityImageFDLabel      = virSecurityDomainSetFDLabelNop,
+    .domainSetSecurityTapFDLabel        = virSecurityDomainSetFDLabelNop,
.domainGetSecurityMountOptions = virSecurityDomainGetMountOptionsNop, 
  };
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 10135ed..0a2a9fe 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -237,6 +237,46 @@ cleanup:
      return mcs;
  }
+static char * 
+virSecuritySELinuxContextAddRange(security_context_t src,
+                                  security_context_t dst)
+{
+    char *str = NULL;
+    char *ret = NULL;
+    context_t srccon = NULL;
+    context_t dstcon = NULL;
+
+    if (!src || !dst)
+        return ret;
+
+    if (!(srccon = context_new(src)) || !(dstcon = context_new(dst))) {
+        virReportSystemError(errno, "%s",
+                             _("unable to allocate security context"));
+        goto cleanup;
+    }
+
+    if (context_range_set(dstcon, context_range_get(srccon)) == -1) {
+        virReportSystemError(errno,
+                             _("unable to set security context range '%s'"), dst);
+        goto cleanup;
+    }
+
+    if (!(str = context_str(dstcon))) {
+        virReportSystemError(errno, "%s",
+                             _("Unable to format SELinux context"));
+        goto cleanup;
+    }
+
+    if (!(ret = strdup(str))) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+cleanup:
+    if (srccon) context_free(srccon);
+    if (dstcon) context_free(dstcon);
+    return ret;
+}
static char * 
  virSecuritySELinuxGenNewContext(const char *basecontext,
@@ -1597,6 +1637,7 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr,
      context_t execcon = NULL;
      context_t proccon = NULL;
      security_context_t scon = NULL;
+    char *str = NULL;
      int rc = -1;
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); 
@@ -1615,13 +1656,6 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr,
          goto done;
      }
- if ( !(execcon = context_new(secdef->label)) ) { 
-        virReportSystemError(errno,
-                             _("unable to allocate socket security context '%s'"),
-                             secdef->label);
-        goto done;
-    }
-
      if (getcon_raw(&scon) == -1) {
          virReportSystemError(errno,
                               _("unable to get current process context '%s'"),
@@ -1629,26 +1663,13 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr,
          goto done;
      }
- if ( !(proccon = context_new(scon)) ) { 
-        virReportSystemError(errno,
-                             _("unable to set socket security context '%s'"),
-                             secdef->label);
-        goto done;
-    }
-
-    if (context_range_set(proccon, context_range_get(execcon)) == -1) {
-        virReportSystemError(errno,
-                             _("unable to set socket security context range '%s'"),
-                             secdef->label);
+    if (!(str = virSecuritySELinuxContextAddRange(secdef->label, scon)))
          goto done;
-    }
- VIR_DEBUG("Setting VM %s socket context %s", 
-              def->name, context_str(proccon));
-    if (setsockcreatecon_raw(context_str(proccon)) == -1) {
+    VIR_DEBUG("Setting VM %s socket context %s", def->name, str);
+    if (setsockcreatecon_raw(str) == -1) {
          virReportSystemError(errno,
-                             _("unable to set socket security context '%s'"),
-                             context_str(proccon));
+                             _("unable to set socket security context '%s'"), str);
          goto done;
      }
@@ -1660,6 +1681,7 @@ done: 
      if (execcon) context_free(execcon);
      if (proccon) context_free(proccon);
      freecon(scon);
+    VIR_FREE(str);
      return rc;
  }
@@ -1869,6 +1891,53 @@ virSecuritySELinuxSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, 
      return virSecuritySELinuxFSetFilecon(fd, secdef->imagelabel);
  }
+static int 
+virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+                                virDomainDefPtr def,
+                                int fd)
+{
+    struct stat buf;
+    security_context_t fcon = NULL;
+    virSecurityLabelDefPtr secdef;
+    char *str = NULL;
+    int rc = -1;
+
+    secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
+    if (secdef == NULL)
+        return rc;
+
+    if (secdef->label == NULL)
+        return 0;
+
+    if (fstat(fd, &buf) < 0) {
+        virReportSystemError(errno, _("cannot stat tap fd %d"), fd);
+        goto cleanup;
+    }
+
+    if ((buf.st_mode & S_IFMT) != S_IFCHR) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("tap fd %d is not character device"), fd);
+        goto cleanup;
+    }
+
+    if (getContext("/dev/tap.*", buf.st_mode, &fcon) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("cannot lookup default selinux label for tap fd %d"), fd);
+        goto cleanup;
+    }
+
+    if (!(str = virSecuritySELinuxContextAddRange(secdef->label, fcon))) {
+        goto cleanup;
+    } else {
+        rc = virSecuritySELinuxFSetFilecon(fd, str);
+    }
+
+cleanup:
+    freecon(fcon);
+    VIR_FREE(str);
+    return rc;
+}
+
  static char *
  virSecuritySELinuxGenImageLabel(virSecurityManagerPtr mgr,
                                  virDomainDefPtr def)
@@ -1969,6 +2038,7 @@ virSecurityDriver virSecurityDriverSELinux = {
      .domainRestoreSavedStateLabel       = virSecuritySELinuxRestoreSavedStateLabel,
.domainSetSecurityImageFDLabel = virSecuritySELinuxSetImageFDLabel, 
+    .domainSetSecurityTapFDLabel        = virSecuritySELinuxSetTapFDLabel,
.domainGetSecurityMountOptions = virSecuritySELinuxGetSecurityMountOptions, 
  };
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 667448f..24de6f2 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -445,6 +445,23 @@ virSecurityStackSetImageFDLabel(virSecurityManagerPtr mgr,
      return rc;
  }
+static int 
+virSecurityStackSetTapFDLabel(virSecurityManagerPtr mgr,
+                              virDomainDefPtr vm,
+                              int fd)
+{
+    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    virSecurityStackItemPtr item = priv->itemsHead;
+    int rc = 0;
+
+    for (; item; item = item->next) {
+        if (virSecurityManagerSetTapFDLabel(item->securityManager, vm, fd) < 0)
+            rc = -1;
+    }
+
+    return rc;
+}
+
  static char *virSecurityStackGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
                                               virDomainDefPtr vm ATTRIBUTE_UNUSED) {
      return NULL;
@@ -509,6 +526,7 @@ virSecurityDriver virSecurityDriverStack = {
      .domainRestoreSavedStateLabel       = virSecurityStackRestoreSavedStateLabel,
.domainSetSecurityImageFDLabel = virSecurityStackSetImageFDLabel, 
+    .domainSetSecurityTapFDLabel        = virSecurityStackSetTapFDLabel,
.domainGetSecurityMountOptions = virSecurityStackGetMountOptions, 
  };


The patch should work on all the tapfd possibilities and from the looks
of it, this is probably the best possibility we can do with the file
descriptor.  Also policies without MCS/MLS ranges should work as well as
policies with the range already set, so ACK from me.

Martin


   Thanks, I am going to push this

   Guannan

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]