[libvirt] [PATCH v2 2/2] selinux: add security selinux function to label tapfd
Guannan Ren
gren at redhat.com
Mon Oct 15 12:51:59 UTC 2012
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
More information about the libvir-list
mailing list