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

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

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?

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.