[libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); } @@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context); +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; } -- 1.7.11.2
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 b278e2c..ec97d78 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, @@ -1605,6 +1645,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); @@ -1623,13 +1664,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'"), @@ -1637,26 +1671,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; } @@ -1668,6 +1689,7 @@ done: if (execcon) context_free(execcon); if (proccon) context_free(proccon); freecon(scon); + VIR_FREE(str); return rc; } @@ -1877,6 +1899,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) @@ -1977,6 +2046,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, }; -- 1.7.11.2
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, }; -- 1.7.11.2
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
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
On 10/15/2012 03: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) ---
This patch is breaking things for me - with it applied, I'm now seeing /dev/net/tun with a label of :MCS that matches the most recently started guest, which prevents other tun clients (like openvpn) from creating their own tapfd. I haven't yet looked closely to see where in this patch you are labeling the wrong file, but it needs to be fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/17/2012 03:19 AM, Eric Blake 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) --- This patch is breaking things for me - with it applied, I'm now seeing /dev/net/tun with a label of :MCS that matches the most recently started guest, which prevents other tun clients (like openvpn) from creating
On 10/15/2012 03:03 AM, Guannan Ren wrote: their own tapfd. I haven't yet looked closely to see where in this patch you are labeling the wrong file, but it needs to be fixed.
Sorry, that is a really stupid mistake I made. I should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE. Patch has sent out. Guannan
On 10/15/2012 09:12 AM, Guannan Ren wrote:
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); }
@@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context);
+#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; }
ACK, Martin
On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); }
@@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context);
+#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; }
I'm not convinced this is safe, because the security drivers can be opened multiple times, eg LXC and QEMU, and this is changing the global static state of the SELinux library. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); }
@@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context);
+#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; }
I'm not convinced this is safe, because the security drivers can be opened multiple times, eg LXC and QEMU, and this is changing the global static state of the SELinux library.
I didn't think the driver is opened for every other driver used. In this case the initialization of the matchpathcon should be dealt with in some other way. Or can't we open the security driver only once? @Guannan: is there a problem this fixes? How come it worked without the init before? Martin
On Mon, Oct 15, 2012 at 12:40:56PM +0200, Martin Kletzander wrote:
On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); }
@@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context);
+#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; }
I'm not convinced this is safe, because the security drivers can be opened multiple times, eg LXC and QEMU, and this is changing the global static state of the SELinux library.
I didn't think the driver is opened for every other driver used. In this case the initialization of the matchpathcon should be dealt with in some other way. Or can't we open the security driver only once?
I say we ignore use of matchpathcon_fini(), and simply call matchpathcon_init() from a VIR_GLOBAL_INIT macro Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/15/2012 06:40 PM, Martin Kletzander wrote:
On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); }
@@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context);
+#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; } I'm not convinced this is safe, because the security drivers can be opened multiple times, eg LXC and QEMU, and this is changing the global static state of the SELinux library.
I didn't think the driver is opened for every other driver used. In this case the initialization of the matchpathcon should be dealt with in some other way. Or can't we open the security driver only once?
@Guannan: is there a problem this fixes? How come it worked without the init before?
Martin
No, there is no a bug/problem to fix, the patch just actively initializes the active context configuration database. If we don't do this, the matchpathcon will do it instead. I am going to add them in a better way later. This patch has nothing to do with the 2/2 patch. Guannan
On Mon, Oct 15, 2012 at 07:03:07PM +0800, Guannan Ren wrote:
On 10/15/2012 06:40 PM, Martin Kletzander wrote:
On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); } @@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context); +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; } I'm not convinced this is safe, because the security drivers can be opened multiple times, eg LXC and QEMU, and this is changing the global static state of the SELinux library.
I didn't think the driver is opened for every other driver used. In this case the initialization of the matchpathcon should be dealt with in some other way. Or can't we open the security driver only once?
@Guannan: is there a problem this fixes? How come it worked without the init before?
Martin
No, there is no a bug/problem to fix, the patch just actively initializes the active context configuration database. If we don't do this, the matchpathcon will do it instead. I am going to add them in a better way later.
There *is* a bug, because you are calling the init/fini methods multiple times and the libselinux code does not handle that usage in a safe manner. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/15/2012 07:35 PM, Daniel P. Berrange wrote:
On Mon, Oct 15, 2012 at 07:03:07PM +0800, Guannan Ren wrote:
On 10/15/2012 06:40 PM, Martin Kletzander wrote:
On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
If we use matchpathcon() to look up selinux context for specific pathname, it'd better actively load file contexts database by matchpathcon_init() and free memory when finished using matchpathcon by matchpathcon_fini(). --- src/security/security_selinux.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10135ed..b278e2c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) { +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_init(NULL) < 0) + VIR_WARN("cannot load selinux active file contexts configuration"); +#endif return virSecuritySELinuxInitialize(mgr); } @@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) VIR_FREE(data->file_context); VIR_FREE(data->content_context); +#ifndef HAVE_SELINUX_LABEL_H + if (matchpathcon_fini() < 0) + VIR_WARN("cannot free allocated memory for selinux"); +#endif return 0; } I'm not convinced this is safe, because the security drivers can be opened multiple times, eg LXC and QEMU, and this is changing the global static state of the SELinux library.
I didn't think the driver is opened for every other driver used. In this case the initialization of the matchpathcon should be dealt with in some other way. Or can't we open the security driver only once?
@Guannan: is there a problem this fixes? How come it worked without the init before?
Martin
No, there is no a bug/problem to fix, the patch just actively initializes the active context configuration database. If we don't do this, the matchpathcon will do it instead. I am going to add them in a better way later. There *is* a bug, because you are calling the init/fini methods multiple times and the libselinux code does not handle that usage in a safe manner.
Daniel
Yes, I learned this from your comments in other emails I means this patch is not to fix particular existing BZ. I will try to add init/fini in a global way later. :)
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Guannan Ren -
Martin Kletzander