[libvirt] [PATCH v4 00/25] Fix and enable owner remembering
This is meant for next release to have the most time possible for testing. Some of the patches were ACKed in v3 already but since they don't make sense on their own I haven't pushed them. v4 of: https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html As usual, you can find (not only these) patches on my github: https://github.com/zippy2/libvirt branch xattr_fixes_v4 diff to v3: - Some new patches (qemusecuritytest and qemusecuritymock) - Some other fixes raised by Cole in review of v3 (like double error reporting and others) - Remembering is done only for paths that cannot be shared between domains. This renders refcounting needless because the refcounter can't ever be greater than one. Nevertheless, I'm keeping it in because in the long run I might come up with a solution to the problem of shared resources and having refcounters might help. Michal Prívozník (25): qemusecuritymock: Mock virProcessRunInFork qemusecuritymock: Fix bit arithmetic qemusecuritymock: Actually set error on failure qemusecuritymock: Introduce and use freePaths() qemusecuritytest: Drop unused variable qemusecuritytest: Use AUTOFREE/AUTOUNREF qemusecuritytest: Fix capabilities loading tools: Slightly rework libvirt_recover_xattrs.sh virSecuritySELinuxRestoreAllLabel: Print @migrated in the debug message too virfile: Make virFileGetXAttr report errors virFileSetXAttr: Report error on failure virFileRemoveXAttr: Report error on failure security: Don't skip label restore on file systems lacking XATTRs security: Document @restore member of transaction list security_dac: Allow caller to suppress owner remembering security_selinux: Allow caller to suppress owner remembering qemusecuritymock: Allow some paths to be not restored security: Don't remember owner for shared resources security: Introduce virSecurityManagerMoveImageMetadata security_util: Introduce virSecurityMoveRememberedLabel security_dac: Implement virSecurityManagerMoveImageMetadata security_selinux: Implement virSecurityManagerMoveImageMetadata qemu_security: Implement qemuSecurityMoveImageMetadata qemu: Move image security metadata on snapshot activity Revert "qemu: Temporary disable owner remembering" docs/news.xml | 13 ++ src/libvirt_private.syms | 2 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 + src/qemu/qemu_blockjob.c | 6 + src/qemu/qemu_conf.c | 4 + src/qemu/qemu_driver.c | 17 +- src/qemu/qemu_security.c | 19 +++ src/qemu/qemu_security.h | 5 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/security/security_dac.c | 171 +++++++++++++++---- src/security/security_driver.h | 5 + src/security/security_manager.c | 39 +++++ src/security/security_manager.h | 4 + src/security/security_nop.c | 10 ++ src/security/security_selinux.c | 263 ++++++++++++++++++++--------- src/security/security_stack.c | 20 +++ src/security/security_util.c | 73 +++++++- src/security/security_util.h | 5 + src/util/virfile.c | 78 +++++++-- src/util/virfile.h | 5 + src/util/virprocess.h | 3 +- tests/qemusecuritymock.c | 76 +++++++-- tests/qemusecuritytest.c | 146 ++++++++++------ tests/qemusecuritytest.h | 4 +- tools/libvirt_recover_xattrs.sh | 50 +++--- 26 files changed, 802 insertions(+), 223 deletions(-) -- 2.21.0
This test is beautiful. It checks if we haven't messed up refcounting on security labels (well, XATTRs where the original owner is stored). It does this by setting up tracking of XATTR setting/removing into a hash table, then calling qemuSecuritySetAllLabel() followed by immediate qemuSecurityRestoreAllLabel() at which point, the hash table must be empty. The test so beautifully written that now matter what you do it won't fail. The reason is that all seclabel work is done in a child process. Therefore, the hash table in the parent is never changed and thus always empty. There are two reasons for forking (only one of them makes sense here though): 1) namespaces - when chown()-ing a file we have to fork() and make the child enter desired namespace, 2) locking - because of exclusive access to XATTRs we lock the files we chown() and this is done in a fork (see 207860927ad for more info). While we want to fork in real world, we don't want that in a test suite. Override virProcessRunInFork() then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.h | 3 ++- tests/qemusecuritymock.c | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 8e5b0c2127..c360bfef98 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -108,7 +108,8 @@ typedef int (*virProcessForkCallback)(pid_t ppid, void *opaque); int virProcessRunInFork(virProcessForkCallback cb, - void *opaque); + void *opaque) + ATTRIBUTE_NOINLINE; int virProcessSetupPrivateMountNS(void); diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index 4edc5c44ad..d170e5da8f 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -416,3 +416,11 @@ int checkPaths(void) virMutexUnlock(&m); return ret; } + + +int +virProcessRunInFork(virProcessForkCallback cb, + void *opaque) +{ + return cb(-1, opaque); +} -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:37AM +0200, Michal Privoznik wrote:
This test is beautiful. It checks if we haven't messed up refcounting on security labels (well, XATTRs where the original owner is stored). It does this by setting up tracking of XATTR setting/removing into a hash table, then calling qemuSecuritySetAllLabel() followed by immediate qemuSecurityRestoreAllLabel() at which point, the hash table must be empty. The test so beautifully written that now matter
s/now/no/
what you do it won't fail. The reason is that all seclabel work is done in a child process. Therefore, the hash table in the parent is never changed and thus always empty.
There are two reasons for forking (only one of them makes sense here though):
1) namespaces - when chown()-ing a file we have to fork() and make the child enter desired namespace, 2) locking - because of exclusive access to XATTRs we lock the files we chown() and this is done in a fork (see 207860927ad for more info).
While we want to fork in real world, we don't want that in a test suite. Override virProcessRunInFork() then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virprocess.h | 3 ++- tests/qemusecuritymock.c | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
One of the functions of this mock is that it spoofs chown() and stat() calls. But it is doing so in a clever way: it stores the new owner on chown() and reports it on subsequent stat(). This is done by using a 32bit unsigned integer where one half is used to store uid the other is for gid. Later, when stat() is called the integer is fetched and split into halves again. Well, my bit operation skills are poor and the code I've written does not do that properly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index d170e5da8f..1ca8bd721f 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -226,7 +226,7 @@ int virFileRemoveXAttr(const char *path, sb->st_gid = DEFAULT_GID; \ } else { \ /* Known path. Set values passed to chown() earlier */ \ - sb->st_uid = *val % 16; \ + sb->st_uid = *val & 0xffff; \ sb->st_gid = *val >> 16; \ } \ \ -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:38AM +0200, Michal Privoznik wrote:
One of the functions of this mock is that it spoofs chown() and stat() calls. But it is doing so in a clever way: it stores the new owner on chown() and reports it on subsequent stat(). This is done by using a 32bit unsigned integer where one half is used to store uid the other is for gid. Later, when stat() is called the integer is fetched and split into halves again. Well, my bit operation skills are poor and the code I've written does not do that properly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
I don't really know what happened when I was writing the original code, but even if error was to be set the corresponding boolean was set to false meaning no error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index 1ca8bd721f..e52a41067a 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -366,7 +366,7 @@ checkOwner(void *payload, fprintf(stderr, "Path %s wasn't restored back to its original owner\n", (const char *) name); - *chown_fail = false; + *chown_fail = true; } return 0; @@ -382,7 +382,7 @@ printXATTR(void *payload, /* The fact that we are in this function means that there are * some XATTRs left behind. This is enough to claim an error. */ - *xattr_fail = false; + *xattr_fail = true; /* Hash table key consists of "$path:$xattr_name", xattr * value is then the value stored in the hash table. */ -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:39AM +0200, Michal Privoznik wrote:
I don't really know what happened when I was writing the original code, but even if error was to be set the corresponding boolean was set to false meaning no error.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Problem with current approach is that if qemuSecuritySetAllLabel() fails, then the @chown_paths and @xattr_paths hash tables are not freed and preserve values already stored there into the next test case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 14 ++++++++++++-- tests/qemusecuritytest.c | 1 + tests/qemusecuritytest.h | 2 ++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index e52a41067a..815bd44d76 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -411,13 +411,23 @@ int checkPaths(void) ret = 0; cleanup: - virHashRemoveAll(chown_paths); - virHashRemoveAll(xattr_paths); virMutexUnlock(&m); return ret; } +void freePaths(void) +{ + virMutexLock(&m); + init_hash(); + + virHashFree(chown_paths); + virHashFree(xattr_paths); + chown_paths = xattr_paths = NULL; + virMutexUnlock(&m); +} + + int virProcessRunInFork(virProcessForkCallback cb, void *opaque) diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 79d4f744b6..9ac953f388 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -125,6 +125,7 @@ testDomain(const void *opaque) unsetenv(ENVVAR); virObjectUnref(vm); virObjectUnref(securityManager); + freePaths(); return ret; } diff --git a/tests/qemusecuritytest.h b/tests/qemusecuritytest.h index a26014b5dc..29c6a9c998 100644 --- a/tests/qemusecuritytest.h +++ b/tests/qemusecuritytest.h @@ -23,4 +23,6 @@ extern int checkPaths(void); +extern void freePaths(void); + #endif /* LIBVIRT_QEMUSECURITYTEST_H */ -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:40AM +0200, Michal Privoznik wrote:
Problem with current approach is that if qemuSecuritySetAllLabel() fails, then the @chown_paths and @xattr_paths hash tables are not freed and preserve values already stored there into the next test case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 14 ++++++++++++-- tests/qemusecuritytest.c | 1 + tests/qemusecuritytest.h | 2 ++ 3 files changed, 15 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The @securityManager variable in testDomain() is unused. Drop it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritytest.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 9ac953f388..993b532329 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -100,7 +100,6 @@ static int testDomain(const void *opaque) { const struct testData *data = opaque; - virSecurityManagerPtr securityManager = NULL; virDomainObjPtr vm = NULL; int ret = -1; @@ -124,7 +123,6 @@ testDomain(const void *opaque) cleanup: unsetenv(ENVVAR); virObjectUnref(vm); - virObjectUnref(securityManager); freePaths(); return ret; } -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:41AM +0200, Michal Privoznik wrote:
The @securityManager variable in testDomain() is unused. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritytest.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This simplifies the code a bit and removes the need for cleanup label in one case. In the other case the label is kept because it's going to be used later. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritytest.c | 50 +++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 993b532329..e7121837a1 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -37,29 +37,29 @@ struct testData { static int prepareObjects(virQEMUDriverPtr driver, const char *xmlname, - virDomainObjPtr *vm) + virDomainObjPtr *vm_ret) { qemuDomainObjPrivatePtr priv; - char *filename = NULL; - char *domxml = NULL; - int ret = -1; + VIR_AUTOUNREF(virDomainObjPtr) vm = NULL; + VIR_AUTOFREE(char *) filename = NULL; + VIR_AUTOFREE(char *) domxml = NULL; if (virAsprintf(&filename, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, xmlname) < 0) return -1; if (virTestLoadFile(filename, &domxml) < 0) - goto cleanup; + return -1; - if (!(*vm = virDomainObjNew(driver->xmlopt))) - goto cleanup; + if (!(vm = virDomainObjNew(driver->xmlopt))) + return -1; - (*vm)->pid = -1; - priv = (*vm)->privateData; + vm->pid = -1; + priv = vm->privateData; priv->chardevStdioLogd = false; priv->rememberOwner = true; if (!(priv->qemuCaps = virQEMUCapsNew())) - goto cleanup; + return -1; virQEMUCapsSetList(priv->qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, @@ -75,24 +75,17 @@ prepareObjects(virQEMUDriverPtr driver, QEMU_CAPS_LAST); if (qemuTestCapsCacheInsert(driver->qemuCapsCache, priv->qemuCaps) < 0) - goto cleanup; + return -1; - if (!((*vm)->def = virDomainDefParseString(domxml, - driver->caps, - driver->xmlopt, - NULL, - 0))) - goto cleanup; + if (!(vm->def = virDomainDefParseString(domxml, + driver->caps, + driver->xmlopt, + NULL, + 0))) + return -1; - ret = 0; - cleanup: - if (ret < 0) { - virObjectUnref(*vm); - *vm = NULL; - } - VIR_FREE(domxml); - VIR_FREE(filename); - return ret; + VIR_STEAL_PTR(*vm_ret, vm); + return 0; } @@ -100,7 +93,7 @@ static int testDomain(const void *opaque) { const struct testData *data = opaque; - virDomainObjPtr vm = NULL; + VIR_AUTOUNREF(virDomainObjPtr) vm = NULL; int ret = -1; if (prepareObjects(data->driver, data->file, &vm) < 0) @@ -109,7 +102,7 @@ testDomain(const void *opaque) /* Mocking is enabled only when this env variable is set. * See mock code for explanation. */ if (setenv(ENVVAR, "1", 0) < 0) - goto cleanup; + return -1; if (qemuSecuritySetAllLabel(data->driver, vm, NULL) < 0) goto cleanup; @@ -122,7 +115,6 @@ testDomain(const void *opaque) ret = 0; cleanup: unsetenv(ENVVAR); - virObjectUnref(vm); freePaths(); return ret; } -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:42AM +0200, Michal Privoznik wrote:
This simplifies the code a bit and removes the need for cleanup label in one case. In the other case the label is kept because it's going to be used later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritytest.c | 50 +++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 29 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Having to enumerate all capabilities that we want domain to have is too verbose and prevents us from adding more tests. Have the domain always have the latest x86_64 capabilities. This means that we have to drop two arm tests, but on the other hand, I'm introducing 50 new cases. I've listed 50 biggest .args files and added those: libvirt.git $ ls -Sr $(find tests/qemuxml2argvdata/ \ -type f -iname "*.x86_64-latest.args") | tail -n 50 Except for two: 1) disk-backing-chains-noindex - this XML has some disks with backing chain. And since set is done on the whole backing chain and restore only on the top layer this would lead to instant test failure. Don't worry, secdrivers will be fixed shortly too and the test case will be added. 2) hostdev-mdev-display-spice-egl-headless - for this XML secdriver tries to find IOMMU group that mdev lives in. Since we are not mocking sysfs access this test case would fail. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritytest.c | 69 ++++++++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 16 deletions(-) diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index e7121837a1..86347f8625 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -43,6 +43,7 @@ prepareObjects(virQEMUDriverPtr driver, VIR_AUTOUNREF(virDomainObjPtr) vm = NULL; VIR_AUTOFREE(char *) filename = NULL; VIR_AUTOFREE(char *) domxml = NULL; + VIR_AUTOFREE(char *) latestCapsFile = NULL; if (virAsprintf(&filename, "%s/qemuxml2argvdata/%s.xml", abs_srcdir, xmlname) < 0) return -1; @@ -58,21 +59,11 @@ prepareObjects(virQEMUDriverPtr driver, priv->chardevStdioLogd = false; priv->rememberOwner = true; - if (!(priv->qemuCaps = virQEMUCapsNew())) + if (!(latestCapsFile = testQemuGetLatestCapsForArch("x86_64", "xml"))) return -1; - virQEMUCapsSetList(priv->qemuCaps, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_VIRTIO_MMIO, - QEMU_CAPS_DEVICE_VIRTIO_RNG, - QEMU_CAPS_OBJECT_GPEX, - QEMU_CAPS_OBJECT_RNG_RANDOM, - QEMU_CAPS_VIRTIO_SCSI, - QEMU_CAPS_LAST); + if (!(priv->qemuCaps = qemuTestParseCapabilitiesArch(VIR_ARCH_X86_64, latestCapsFile))) + return -1; if (qemuTestCapsCacheInsert(driver->qemuCapsCache, priv->qemuCaps) < 0) return -1; @@ -148,11 +139,57 @@ mymain(void) ret = -1; \ } while (0) + DO_TEST_DOMAIN("acpi-table"); + DO_TEST_DOMAIN("channel-unix-guestfwd"); + DO_TEST_DOMAIN("console-virtio-unix"); + DO_TEST_DOMAIN("controller-virtio-scsi"); + DO_TEST_DOMAIN("disk-aio"); + DO_TEST_DOMAIN("disk-cache"); + DO_TEST_DOMAIN("disk-cdrom"); + DO_TEST_DOMAIN("disk-cdrom-bus-other"); + DO_TEST_DOMAIN("disk-cdrom-network"); + DO_TEST_DOMAIN("disk-cdrom-tray"); + DO_TEST_DOMAIN("disk-copy_on_read"); + DO_TEST_DOMAIN("disk-detect-zeroes"); + DO_TEST_DOMAIN("disk-error-policy"); + DO_TEST_DOMAIN("disk-floppy"); + DO_TEST_DOMAIN("disk-floppy-q35-2_11"); + DO_TEST_DOMAIN("disk-floppy-q35-2_9"); + DO_TEST_DOMAIN("disk-network-gluster"); + DO_TEST_DOMAIN("disk-network-iscsi"); + DO_TEST_DOMAIN("disk-network-nbd"); + DO_TEST_DOMAIN("disk-network-rbd"); + DO_TEST_DOMAIN("disk-network-sheepdog"); + DO_TEST_DOMAIN("disk-network-source-auth"); + DO_TEST_DOMAIN("disk-network-tlsx509"); + DO_TEST_DOMAIN("disk-readonly-disk"); + DO_TEST_DOMAIN("disk-scsi"); + DO_TEST_DOMAIN("disk-scsi-device-auto"); + DO_TEST_DOMAIN("disk-shared"); DO_TEST_DOMAIN("disk-virtio"); + DO_TEST_DOMAIN("disk-virtio-scsi-reservations"); + DO_TEST_DOMAIN("graphics-vnc-tls-secret"); + DO_TEST_DOMAIN("hugepages-nvdimm"); + DO_TEST_DOMAIN("iothreads-virtio-scsi-pci"); + DO_TEST_DOMAIN("memory-hotplug-nvdimm"); + DO_TEST_DOMAIN("memory-hotplug-nvdimm-access"); + DO_TEST_DOMAIN("memory-hotplug-nvdimm-align"); + DO_TEST_DOMAIN("memory-hotplug-nvdimm-label"); + DO_TEST_DOMAIN("memory-hotplug-nvdimm-pmem"); + DO_TEST_DOMAIN("memory-hotplug-nvdimm-readonly"); + DO_TEST_DOMAIN("net-vhostuser"); + DO_TEST_DOMAIN("os-firmware-bios"); + DO_TEST_DOMAIN("os-firmware-efi"); + DO_TEST_DOMAIN("os-firmware-efi-secboot"); DO_TEST_DOMAIN("pci-bridge-many-disks"); - DO_TEST_DOMAIN("arm-virt-virtio"); - DO_TEST_DOMAIN("aarch64-virtio-pci-manual-addresses"); - DO_TEST_DOMAIN("acpi-table"); + DO_TEST_DOMAIN("tseg-explicit-size"); + DO_TEST_DOMAIN("usb-redir-unix"); + DO_TEST_DOMAIN("virtio-non-transitional"); + DO_TEST_DOMAIN("virtio-transitional"); + DO_TEST_DOMAIN("x86_64-pc-graphics"); + DO_TEST_DOMAIN("x86_64-pc-headless"); + DO_TEST_DOMAIN("x86_64-q35-graphics"); + DO_TEST_DOMAIN("x86_64-q35-headless"); cleanup: qemuTestDriverFree(&driver); -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:43AM +0200, Michal Privoznik wrote:
Having to enumerate all capabilities that we want domain to have is too verbose and prevents us from adding more tests. Have the domain always have the latest x86_64 capabilities. This means that we have to drop two arm tests, but on the other hand, I'm introducing 50 new cases. I've listed 50 biggest .args files and added those:
libvirt.git $ ls -Sr $(find tests/qemuxml2argvdata/ \ -type f -iname "*.x86_64-latest.args") | tail -n 50
Except for two: 1) disk-backing-chains-noindex - this XML has some disks with backing chain. And since set is done on the whole backing chain and restore only on the top layer this would lead to instant test failure. Don't worry, secdrivers will be fixed shortly too and the test case will be added.
2) hostdev-mdev-display-spice-egl-headless - for this XML secdriver tries to find IOMMU group that mdev lives in. Since we are not mocking sysfs access this test case would fail.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritytest.c | 69 ++++++++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 16 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Firstly, there's no reason to enumerate all XATTRs since they differ only in the prefix and we can construct them in a loop. Secondly, and more importantly, the script was still looking for just one prefix "trusted.libvirt.security" even on FreeBSD. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- tools/libvirt_recover_xattrs.sh | 50 ++++++++++++++++----------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh index 69dfca0160..58f02f8dfb 100755 --- a/tools/libvirt_recover_xattrs.sh +++ b/tools/libvirt_recover_xattrs.sh @@ -23,14 +23,16 @@ EOF QUIET=0 DRY_RUN=0 -P="/" +DIR="/" # So far only qemu and lxc drivers use security driver. URI=("qemu:///system" - "qemu:///session" "lxc:///system") -LIBVIRT_XATTR_PREFIX="trusted.libvirt.security" +# On Linux we use 'trusted' namespace, on FreeBSD we use 'system' +# as there is no 'trusted'. +LIBVIRT_XATTR_PREFIXES=("trusted.libvirt.security" + "system.libvirt.security") if [ `whoami` != "root" ]; then die "Must be run as root" @@ -57,7 +59,7 @@ done shift $((OPTIND - 1)) if [ $# -gt 0 ]; then - P=$1 + DIR=$1 fi if [ ${DRY_RUN} -eq 0 ]; then @@ -69,28 +71,26 @@ if [ ${DRY_RUN} -eq 0 ]; then fi -# On Linux we use 'trusted' namespace, on FreeBSD we use 'system' -# as there is no 'trusted'. -XATTRS=("trusted.libvirt.security.dac" - "trusted.libvirt.security.ref_dac" - "trusted.libvirt.security.selinux" - "trusted.libvirt.security.ref_selinux", - "system.libvirt.security.dac" - "system.libvirt.security.ref_dac" - "system.libvirt.security.selinux" - "system.libvirt.security.ref_selinux") +declare -a XATTRS +for i in "dac" "selinux"; do + for p in ${LIBVIRT_XATTR_PREFIXES[@]}; do + XATTRS+=("$p.$i" "$p.ref_$i") + done +done -for i in $(getfattr -R -d -m ${LIBVIRT_XATTR_PREFIX} --absolute-names ${P} 2>/dev/null | grep "^# file:" | cut -d':' -f 2); do - if [ ${DRY_RUN} -ne 0 ]; then - echo $i - getfattr -d -m ${LIBVIRT_XATTR_PREFIX} $i - continue - fi +for p in ${LIBVIRT_XATTR_PREFIXES[*]}; do + for i in $(getfattr -R -d -m ${p} --absolute-names ${DIR} 2>/dev/null | grep "^# file:" | cut -d':' -f 2); do + echo $i; + if [ ${DRY_RUN} -ne 0 ]; then + getfattr -d -m $p --absolute-names $i | grep -v "^# file:" + continue + fi - if [ ${QUIET} -eq 0 ]; then - echo "Fixing $i"; - fi - for x in ${XATTRS[*]}; do - setfattr -x $x $i + if [ ${QUIET} -eq 0 ]; then + echo "Fixing $i"; + fi + for x in ${XATTRS[*]}; do + setfattr -x $x $i + done done done -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:44AM +0200, Michal Privoznik wrote:
Firstly, there's no reason to enumerate all XATTRs since they differ only in the prefix and we can construct them in a loop.
Secondly, and more importantly, the script was still looking for just one prefix "trusted.libvirt.security" even on FreeBSD.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- tools/libvirt_recover_xattrs.sh | 50 ++++++++++++++++----------------- 1 file changed, 25 insertions(+), 25 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Just like it's DAC counterpart is doing, virSecuritySELinuxRestoreAllLabel() could print @migrated in the debug message. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 094272e084..ec8d407351 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2597,7 +2597,7 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, size_t i; int rc = 0; - VIR_DEBUG("Restoring security label on %s", def->name); + VIR_DEBUG("Restoring security label on %s migrated=%d", def->name, migrated); secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:45AM +0200, Michal Privoznik wrote:
Just like it's DAC counterpart is doing, virSecuritySELinuxRestoreAllLabel() could print @migrated in the debug message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The way that security drivers use XATTR is kind of verbose. If error reporting was left for caller then the caller would end up even more verbose. There are two places where we do not want to report error if virFileGetXAttr fails. Therefore virFileGetXAttrQuiet is introduced as an alternative that doesn't report errors. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/security/security_util.c | 4 ++-- src/util/virfile.c | 42 ++++++++++++++++++++++++++++++------ src/util/virfile.h | 5 +++++ tests/qemusecuritymock.c | 6 +++--- 5 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a03cf0b645..5368392882 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1885,6 +1885,7 @@ virFileGetHugepageSize; virFileGetMountReverseSubtree; virFileGetMountSubtree; virFileGetXAttr; +virFileGetXAttrQuiet; virFileInData; virFileIsAbsPath; virFileIsCDROM; diff --git a/src/security/security_util.c b/src/security/security_util.c index bfa78c6cca..f09a18a623 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -123,7 +123,7 @@ virSecurityGetRememberedLabel(const char *name, if (!(ref_name = virSecurityGetRefCountAttrName(name))) goto cleanup; - if (virFileGetXAttr(path, ref_name, &value) < 0) { + if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) { ret = 0; } else { @@ -208,7 +208,7 @@ virSecuritySetRememberedLabel(const char *name, if (!(ref_name = virSecurityGetRefCountAttrName(name))) goto cleanup; - if (virFileGetXAttr(path, ref_name, &value) < 0) { + if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENOTSUP) { ret = 0; goto cleanup; diff --git a/src/util/virfile.c b/src/util/virfile.c index f7415cf633..00f69dce69 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4364,7 +4364,7 @@ virFileWaitForExists(const char *path, #if HAVE_LIBATTR /** - * virFileGetXAttr; + * virFileGetXAttrQuiet; * @path: a filename * @name: name of xattr * @value: read value @@ -4376,9 +4376,9 @@ virFileWaitForExists(const char *path, * -1 otherwise (with errno set). */ int -virFileGetXAttr(const char *path, - const char *name, - char **value) +virFileGetXAttrQuiet(const char *path, + const char *name, + char **value) { char *buf = NULL; int ret = -1; @@ -4451,9 +4451,9 @@ virFileRemoveXAttr(const char *path, #else /* !HAVE_LIBATTR */ int -virFileGetXAttr(const char *path ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED, - char **value ATTRIBUTE_UNUSED) +virFileGetXAttrQuiet(const char *path ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + char **value ATTRIBUTE_UNUSED) { errno = ENOSYS; return -1; @@ -4477,3 +4477,31 @@ virFileRemoveXAttr(const char *path ATTRIBUTE_UNUSED, } #endif /* HAVE_LIBATTR */ + +/** + * virFileGetXAttr; + * @path: a filename + * @name: name of xattr + * @value: read value + * + * Reads xattr with @name for given @path and stores it into + * @value. Caller is responsible for freeing @value. + * + * Returns: 0 on success, + * -1 otherwise (with errno set AND error reported). + */ +int +virFileGetXAttr(const char *path, + const char *name, + char **value) +{ + int ret; + + if ((ret = virFileGetXAttrQuiet(path, name, value)) < 0) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + name, path); + } + + return ret; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index 641960e2ca..1cd6dfc789 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -384,6 +384,11 @@ int virFileGetXAttr(const char *path, char **value) ATTRIBUTE_NOINLINE; +int virFileGetXAttrQuiet(const char *path, + const char *name, + char **value) + ATTRIBUTE_NOINLINE; + int virFileSetXAttr(const char *path, const char *name, const char *value) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index 815bd44d76..e219a9f023 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -126,9 +126,9 @@ get_key(const char *path, int -virFileGetXAttr(const char *path, - const char *name, - char **value) +virFileGetXAttrQuiet(const char *path, + const char *name, + char **value) { int ret = -1; char *key; -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:46AM +0200, Michal Privoznik wrote:
The way that security drivers use XATTR is kind of verbose. If error reporting was left for caller then the caller would end up even more verbose.
There are two places where we do not want to report error if virFileGetXAttr fails. Therefore virFileGetXAttrQuiet is introduced as an alternative that doesn't report errors.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/security/security_util.c | 4 ++-- src/util/virfile.c | 42 ++++++++++++++++++++++++++++++------ src/util/virfile.h | 5 +++++ tests/qemusecuritymock.c | 6 +++--- 5 files changed, 46 insertions(+), 12 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
It's better to have the function report errors, because none of the callers does. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/util/virfile.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 00f69dce69..75ec9e0bd8 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4421,14 +4421,21 @@ virFileGetXAttrQuiet(const char *path, * Sets xattr of @name and @value on @path. * * Returns: 0 on success, - * -1 otherwise (with errno set). + * -1 otherwise (with errno set AND error reported). */ int virFileSetXAttr(const char *path, const char *name, const char *value) { - return setxattr(path, name, value, strlen(value), 0); + if (setxattr(path, name, value, strlen(value), 0) < 0) { + virReportSystemError(errno, + _("Unable to set XATTR %s on %s"), + name, path); + return -1; + } + + return 0; } /** @@ -4460,11 +4467,14 @@ virFileGetXAttrQuiet(const char *path ATTRIBUTE_UNUSED, } int -virFileSetXAttr(const char *path ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED, +virFileSetXAttr(const char *path, + const char *name, const char *value ATTRIBUTE_UNUSED) { errno = ENOSYS; + virReportSystemError(errno, + _("Unable to set XATTR %s on %s"), + name, path); return -1; } -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:47AM +0200, Michal Privoznik wrote:
It's better to have the function report errors, because none of the callers does.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/util/virfile.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
It's better to have the function report errors, because none of the callers does. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/util/virfile.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 75ec9e0bd8..b351f72bef 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4446,13 +4446,20 @@ virFileSetXAttr(const char *path, * Remove xattr of @name on @path. * * Returns: 0 on success, - * -1 otherwise (with errno set). + * -1 otherwise (with errno set AND error reported). */ int virFileRemoveXAttr(const char *path, const char *name) { - return removexattr(path, name); + if (removexattr(path, name) < 0) { + virReportSystemError(errno, + _("Unable to remove XATTR %s on %s"), + name, path); + return -1; + } + + return 0; } #else /* !HAVE_LIBATTR */ @@ -4479,10 +4486,13 @@ virFileSetXAttr(const char *path, } int -virFileRemoveXAttr(const char *path ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED) +virFileRemoveXAttr(const char *path, + const char *name) { errno = ENOSYS; + virReportSystemError(errno, + _("Unable to remove XATTR %s on %s"), + name, path); return -1; } -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:48AM +0200, Michal Privoznik wrote:
It's better to have the function report errors, because none of the callers does.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/util/virfile.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The way that virSecurityDACRecallLabel is currently written is that if XATTRs are not supported for given path to the caller this is not different than if the path is still in use. The value of 1 is returned which makes secdrivers skip label restore. This is clearly a bug as we are not restoring labels on say NFS even though previously we were. Strictly speaking, changes to virSecurityDACRememberLabel are not needed, but they are done anyway so that getter and setter behave in the same fashion. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 18 ++++++++++++------ src/security/security_selinux.c | 21 +++++++++++++++------ src/security/security_util.c | 6 ++++-- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 3c21dbbddb..300c383dd5 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -458,10 +458,11 @@ virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, { char *label; int ret = -1; + int rv; - if (virSecurityGetRememberedLabel(SECURITY_DAC_NAME, - path, &label) < 0) - goto cleanup; + rv = virSecurityGetRememberedLabel(SECURITY_DAC_NAME, path, &label); + if (rv < 0) + return rv; if (!label) return 1; @@ -760,7 +761,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, } refcount = virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid); - if (refcount < 0) { + if (refcount == -2) { + /* Not supported. Don't error though. */ + } else if (refcount < 0) { return -1; } else if (refcount > 1) { /* Refcount is greater than 1 which means that there @@ -827,10 +830,13 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, if (recall && path) { rv = virSecurityDACRecallLabel(priv, path, &uid, &gid); - if (rv < 0) + if (rv == -2) { + /* Not supported. Don't error though. */ + } else if (rv < 0) { return -1; - if (rv > 0) + } else if (rv > 0) { return 0; + } } VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ec8d407351..ff54d47e23 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -207,9 +207,11 @@ static int virSecuritySELinuxRecallLabel(const char *path, security_context_t *con) { - if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, - path, con) < 0) - return -1; + int rv; + + rv = virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, path, con); + if (rv < 0) + return rv; if (!*con) return 1; @@ -1337,7 +1339,9 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, if (econ) { refcount = virSecuritySELinuxRememberLabel(path, econ); - if (refcount < 0) { + if (refcount == -2) { + /* Not supported. Don't error though. */ + } else if (refcount < 0) { goto cleanup; } else if (refcount > 1) { /* Refcount is greater than 1 which means that there @@ -1485,13 +1489,18 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, } if (recall) { - if ((rc = virSecuritySELinuxRecallLabel(newpath, &fcon)) < 0) { + rc = virSecuritySELinuxRecallLabel(newpath, &fcon); + if (rc == -2) { + /* Not supported. Lookup the default label below. */ + } else if (rc < 0) { goto cleanup; } else if (rc > 0) { ret = 0; goto cleanup; } - } else { + } + + if (!recall || rc == -2) { if (stat(newpath, &buf) != 0) { VIR_WARN("cannot stat %s: %s", newpath, virStrerror(errno, ebuf, sizeof(ebuf))); diff --git a/src/security/security_util.c b/src/security/security_util.c index f09a18a623..3c24d7cded 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -105,6 +105,7 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED) * zero) and returns zero. * * Returns: 0 on success, + * -2 if underlying file system doesn't support XATTRs, * -1 otherwise (with error reported) */ int @@ -125,7 +126,7 @@ virSecurityGetRememberedLabel(const char *name, if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) { - ret = 0; + ret = -2; } else { virReportSystemError(errno, _("Unable to get XATTR %s on %s"), @@ -192,6 +193,7 @@ virSecurityGetRememberedLabel(const char *name, * See also virSecurityGetRememberedLabel. * * Returns: the new refcount value on success, + * -2 if underlying file system doesn't support XATTRs, * -1 otherwise (with error reported) */ int @@ -210,7 +212,7 @@ virSecuritySetRememberedLabel(const char *name, if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENOTSUP) { - ret = 0; + ret = -2; goto cleanup; } else if (errno != ENODATA) { virReportSystemError(errno, -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:49AM +0200, Michal Privoznik wrote:
The way that virSecurityDACRecallLabel is currently written is that if XATTRs are not supported for given path to the caller this is not different than if the path is still in use. The value of 1 is returned which makes secdrivers skip label restore. This is clearly a bug as we are not restoring labels on say NFS even though previously we were.
Strictly speaking, changes to virSecurityDACRememberLabel are not needed, but they are done anyway so that getter and setter behave in the same fashion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 18 ++++++++++++------ src/security/security_selinux.c | 21 +++++++++++++++------ src/security/security_util.c | 6 ++++-- 3 files changed, 31 insertions(+), 14 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Both DAC and SELinux drivers support transactions. Each item on the transaction list consists of various variables and @restore is one of them. Document it so that as the list of variables grow it's easier to spot which variable does what. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 5 ++++- src/security/security_selinux.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 300c383dd5..c19421fa8f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -79,7 +79,7 @@ struct _virSecurityDACChownItem { const virStorageSource *src; uid_t uid; gid_t gid; - bool restore; + bool restore; /* Whether current operation is 'set' or 'restore' */ }; typedef struct _virSecurityDACChownList virSecurityDACChownList; @@ -155,8 +155,11 @@ virSecurityDACChownListFree(void *opaque) * @src: disk source to chown * @uid: user ID * @gid: group ID + * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @restore should be true if the operation is restoring + * seclabel and false otherwise. * * Returns: 1 in case of successful append * 0 if there is no transaction enabled diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ff54d47e23..38f4e3afd8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -82,7 +82,7 @@ struct _virSecuritySELinuxContextItem { char *path; char *tcon; bool optional; - bool restore; + bool restore; /* Whether current operation is 'set' or 'restore' */ }; typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList; @@ -168,8 +168,11 @@ virSecuritySELinuxContextListFree(void *opaque) * @path: Path to chown * @tcon: target context * @optional: true if setting @tcon is optional + * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @restore should be true if the operation is restoring + * seclabel and false otherwise. * * Returns: 1 in case of successful append * 0 if there is no transaction enabled -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:50AM +0200, Michal Privoznik wrote:
Both DAC and SELinux drivers support transactions. Each item on the transaction list consists of various variables and @restore is one of them. Document it so that as the list of variables grow it's easier to spot which variable does what.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 5 ++++- src/security/security_selinux.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
One caller in particular (virSecurityDACSetImageLabelInternal) will want to have the feature turned on only in some cases. Introduce @remember member to _virSecurityDACChownItem to track whether caller wants to do owner remembering or not. The actual remembering is then enabled if both caller wanted it and the feature is turned on in the config file. Technically, we could skip over paths that don't have remember enabled when creating a list of paths to lock. We won't touch their XATTRs after all. Well, I rather play it safe and keep them on the locking list for now. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 63 ++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index c19421fa8f..a39dae5226 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -79,6 +79,7 @@ struct _virSecurityDACChownItem { const virStorageSource *src; uid_t uid; gid_t gid; + bool remember; /* Whether owner remembering should be done for @path/@src */ bool restore; /* Whether current operation is 'set' or 'restore' */ }; @@ -100,6 +101,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, const virStorageSource *src, uid_t uid, gid_t gid, + bool remember, bool restore) { int ret = -1; @@ -116,6 +118,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, item->src = src; item->uid = uid; item->gid = gid; + item->remember = remember; item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) @@ -155,9 +158,12 @@ virSecurityDACChownListFree(void *opaque) * @src: disk source to chown * @uid: user ID * @gid: group ID + * @remember: if the original owner should be recorded/recalled * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @remember should be true if caller wishes to record/recall + * the original owner of @path/@src. * The @restore should be true if the operation is restoring * seclabel and false otherwise. * @@ -170,13 +176,15 @@ virSecurityDACTransactionAppend(const char *path, const virStorageSource *src, uid_t uid, gid_t gid, + bool remember, bool restore) { virSecurityDACChownListPtr list = virThreadLocalGet(&chownList); if (!list) return 0; - if (virSecurityDACChownListAppend(list, path, src, uid, gid, restore) < 0) + if (virSecurityDACChownListAppend(list, path, src, + uid, gid, remember, restore) < 0) return -1; return 1; @@ -235,6 +243,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (i = 0; i < list->nItems; i++) { virSecurityDACChownItemPtr item = list->items[i]; + const bool remember = item->remember && list->lock; if (!item->restore) { rv = virSecurityDACSetOwnership(list->manager, @@ -242,12 +251,12 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, item->path, item->uid, item->gid, - list->lock); + remember); } else { rv = virSecurityDACRestoreFileLabelInternal(list->manager, item->src, item->path, - list->lock); + remember); } if (rv < 0) @@ -256,12 +265,13 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (; rv < 0 && i > 0; i--) { virSecurityDACChownItemPtr item = list->items[i - 1]; + const bool remember = item->remember && list->lock; if (!item->restore) { virSecurityDACRestoreFileLabelInternal(list->manager, item->src, item->path, - list->lock); + remember); } else { VIR_WARN("Ignoring failed restore attempt on %s", NULLSTR(item->src ? item->src->path : item->path)); @@ -752,7 +762,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ - if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid, false)) < 0) + if ((rc = virSecurityDACTransactionAppend(path, src, + uid, gid, remember, false)) < 0) return -1; else if (rc > 0) return 0; @@ -826,7 +837,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ - if ((rv = virSecurityDACTransactionAppend(path, src, uid, gid, true)) < 0) + if ((rv = virSecurityDACTransactionAppend(path, src, uid, gid, recall, true)) < 0) return -1; else if (rv > 0) return 0; @@ -853,7 +864,7 @@ static int virSecurityDACRestoreFileLabel(virSecurityManagerPtr mgr, const char *path) { - return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path, false); + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path, true); } @@ -900,7 +911,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnership(mgr, src, NULL, user, group, false); + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, true); } @@ -967,7 +978,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, false); + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true); } @@ -995,7 +1006,7 @@ virSecurityDACSetHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, file, user, group, false); + return virSecurityDACSetOwnership(mgr, NULL, file, user, group, true); } @@ -1371,7 +1382,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group, false); + user, group, true); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1379,12 +1390,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, false) < 0 || - virSecurityDACSetOwnership(mgr, NULL, out, user, group, false) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, true) < 0 || + virSecurityDACSetOwnership(mgr, NULL, out, user, group, true) < 0) goto done; } else if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group, false) < 0) { + user, group, true) < 0) { goto done; } ret = 0; @@ -1399,7 +1410,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, * and passed via FD */ if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, - user, group, false) < 0) + user, group, true) < 0) goto done; } ret = 0; @@ -1589,7 +1600,7 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - if (virSecurityDACSetOwnership(mgr, NULL, rendernode, user, group, false) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, rendernode, user, group, true) < 0) return -1; return 0; @@ -1632,7 +1643,7 @@ virSecurityDACSetInputLabel(virSecurityManagerPtr mgr, ret = virSecurityDACSetOwnership(mgr, NULL, input->source.evdev, - user, group, false); + user, group, true); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1837,7 +1848,7 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, ret = virSecurityDACSetOwnership(mgr, NULL, mem->nvdimmPath, - user, group, false); + user, group, true); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1874,7 +1885,7 @@ virSecurityDACSetSEVLabel(virSecurityManagerPtr mgr, return -1; if (virSecurityDACSetOwnership(mgr, NULL, DEV_SEV, - user, group, false) < 0) + user, group, true) < 0) return -1; return 0; @@ -1961,31 +1972,31 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, if (def->os.loader && def->os.loader->nvram && virSecurityDACSetOwnership(mgr, NULL, def->os.loader->nvram, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.kernel && virSecurityDACSetOwnership(mgr, NULL, def->os.kernel, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.initrd && virSecurityDACSetOwnership(mgr, NULL, def->os.initrd, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.dtb && virSecurityDACSetOwnership(mgr, NULL, def->os.dtb, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.slic_table && virSecurityDACSetOwnership(mgr, NULL, def->os.slic_table, - user, group, false) < 0) + user, group, true) < 0) return -1; return 0; @@ -2007,7 +2018,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group, false); + return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group, true); } @@ -2327,7 +2338,7 @@ virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, path, user, group, false); + return virSecurityDACSetOwnership(mgr, NULL, path, user, group, true); } virSecurityDriver virSecurityDriverDAC = { -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:51AM +0200, Michal Privoznik wrote:
One caller in particular (virSecurityDACSetImageLabelInternal) will want to have the feature turned on only in some cases. Introduce @remember member to _virSecurityDACChownItem to track whether caller wants to do owner remembering or not. The actual remembering is then enabled if both caller wanted it and the feature is turned on in the config file.
Technically, we could skip over paths that don't have remember enabled when creating a list of paths to lock. We won't touch their XATTRs after all. Well, I rather play it safe and keep them on the locking list for now.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 63 ++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 26 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Just like previous commit allowed to enable or disable owner remembering for each individual path, do the same for SELinux driver. This is going to be needed in the next commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 163 ++++++++++++++++++-------------- 1 file changed, 94 insertions(+), 69 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 38f4e3afd8..3ac3b83e45 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -82,6 +82,7 @@ struct _virSecuritySELinuxContextItem { char *path; char *tcon; bool optional; + bool remember; /* Whether owner remembering should be done for @path/@src */ bool restore; /* Whether current operation is 'set' or 'restore' */ }; @@ -122,6 +123,7 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *path, const char *tcon, bool optional, + bool remember, bool restore) { int ret = -1; @@ -134,6 +136,7 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, goto cleanup; item->optional = optional; + item->remember = remember; item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) @@ -168,9 +171,12 @@ virSecuritySELinuxContextListFree(void *opaque) * @path: Path to chown * @tcon: target context * @optional: true if setting @tcon is optional + * @remember: if the original owner should be recorded/recalled * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @remember should be true if caller wishes to record/recall + * the original owner of @path/@src. * The @restore should be true if the operation is restoring * seclabel and false otherwise. * @@ -182,6 +188,7 @@ static int virSecuritySELinuxTransactionAppend(const char *path, const char *tcon, bool optional, + bool remember, bool restore) { virSecuritySELinuxContextListPtr list; @@ -190,7 +197,8 @@ virSecuritySELinuxTransactionAppend(const char *path, if (!list) return 0; - if (virSecuritySELinuxContextListAppend(list, path, tcon, optional, restore) < 0) + if (virSecuritySELinuxContextListAppend(list, path, tcon, + optional, remember, restore) < 0) return -1; return 1; @@ -276,17 +284,18 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, rv = 0; for (i = 0; i < list->nItems; i++) { virSecuritySELinuxContextItemPtr item = list->items[i]; + const bool remember = item->remember && list->lock; if (!item->restore) { rv = virSecuritySELinuxSetFileconHelper(list->manager, item->path, item->tcon, item->optional, - list->lock); + remember); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, item->path, - list->lock); + remember); } if (rv < 0) @@ -295,11 +304,12 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (; rv < 0 && i > 0; i--) { virSecuritySELinuxContextItemPtr item = list->items[i - 1]; + const bool remember = item->remember && list->lock; if (!item->restore) { virSecuritySELinuxRestoreFileLabel(list->manager, item->path, - list->lock); + remember); } else { VIR_WARN("Ignoring failed restore attempt on %s", item->path); } @@ -1326,7 +1336,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, int rc; int ret = -1; - if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional, false)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, + optional, remember, false)) < 0) return -1; else if (rc > 0) return 0; @@ -1389,16 +1400,20 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, static int virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, - const char *path, const char *tcon) + const char *path, + const char *tcon, + bool remember) { - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, false); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, remember); } static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, - const char *path, const char *tcon) + const char *path, + const char *tcon, + bool remember) { - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, false); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, remember); } static int @@ -1484,7 +1499,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto cleanup; } - if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0) { + if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, + false, recall, true)) < 0) { goto cleanup; } else if (rc > 0) { ret = 0; @@ -1545,7 +1561,7 @@ virSecuritySELinuxSetInputLabel(virSecurityManagerPtr mgr, switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: if (virSecuritySELinuxSetFilecon(mgr, input->source.evdev, - seclabel->imagelabel) < 0) + seclabel->imagelabel, true) < 0) return -1; break; @@ -1574,7 +1590,7 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManagerPtr mgr, switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, false); + rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, true); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1602,7 +1618,7 @@ virSecuritySELinuxSetMemoryLabel(virSecurityManagerPtr mgr, return 0; if (virSecuritySELinuxSetFilecon(mgr, mem->nvdimmPath, - seclabel->imagelabel) < 0) + seclabel->imagelabel, true) < 0) return -1; break; @@ -1630,7 +1646,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr, if (!seclabel || !seclabel->relabel) return 0; - ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath, true); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1661,14 +1677,14 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); if (rc < 0) return -1; if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { rc = virSecuritySELinuxSetFilecon(mgr, cancel_path, - seclabel->imagelabel); + seclabel->imagelabel, true); VIR_FREE(cancel_path); if (rc < 0) { virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm); @@ -1680,7 +1696,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: tpmdev = tpm->data.emulator.source.data.nix.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); if (rc < 0) return -1; break; @@ -1709,10 +1725,10 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, false); + rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, true); if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { - if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, true) < 0) rc = -1; VIR_FREE(cancel_path); } @@ -1779,7 +1795,7 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecuritySELinuxRestoreFileLabel(mgr, src->path, false); + return virSecuritySELinuxRestoreFileLabel(mgr, src->path, true); } @@ -1822,32 +1838,38 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, if (!disk_seclabel->relabel) return 0; - ret = virSecuritySELinuxSetFilecon(mgr, src->path, disk_seclabel->label); + ret = virSecuritySELinuxSetFilecon(mgr, src->path, + disk_seclabel->label, true); } else if (parent_seclabel && (!parent_seclabel->relabel || parent_seclabel->label)) { if (!parent_seclabel->relabel) return 0; - ret = virSecuritySELinuxSetFilecon(mgr, src->path, parent_seclabel->label); + ret = virSecuritySELinuxSetFilecon(mgr, src->path, + parent_seclabel->label, true); } else if (!parent || parent == src) { if (src->shared) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - data->file_context); + data->file_context, + true); } else if (src->readonly) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - data->content_context); + data->content_context, + true); } else if (secdef->imagelabel) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - secdef->imagelabel); + secdef->imagelabel, + true); } else { ret = 0; } } else { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - data->content_context); + data->content_context, + true); } if (ret == 1 && !disk_seclabel) { @@ -1900,7 +1922,7 @@ virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque) secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) return 0; - return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel); + return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel, true); } static int @@ -1932,13 +1954,13 @@ virSecuritySELinuxSetSCSILabel(virSCSIDevicePtr dev, if (virSCSIDeviceGetShareable(dev)) return virSecuritySELinuxSetFileconOptional(mgr, file, - data->file_context); + data->file_context, true); else if (virSCSIDeviceGetReadonly(dev)) return virSecuritySELinuxSetFileconOptional(mgr, file, - data->content_context); + data->content_context, true); else return virSecuritySELinuxSetFileconOptional(mgr, file, - secdef->imagelabel); + secdef->imagelabel, true); } static int @@ -2093,7 +2115,7 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0) return -1; } - ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel); + ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel, true); VIR_FREE(path); break; } @@ -2107,7 +2129,7 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0) return -1; } - ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel); + ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel, true); VIR_FREE(path); break; } @@ -2153,7 +2175,7 @@ virSecuritySELinuxRestorePCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } static int @@ -2163,7 +2185,7 @@ virSecuritySELinuxRestoreUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } @@ -2180,7 +2202,7 @@ virSecuritySELinuxRestoreSCSILabel(virSCSIDevicePtr dev, if (virSCSIDeviceGetShareable(dev) || virSCSIDeviceGetReadonly(dev)) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } static int @@ -2190,7 +2212,7 @@ virSecuritySELinuxRestoreHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } @@ -2294,7 +2316,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) goto done; - ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, true); VIR_FREE(vfiodev); break; @@ -2328,7 +2350,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); VIR_FREE(path); break; } @@ -2342,7 +2364,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); VIR_FREE(path); break; } @@ -2420,14 +2442,16 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecuritySELinuxSetFilecon(mgr, dev_source->data.file.path, - imagelabel); + imagelabel, + true); break; case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { if (virSecuritySELinuxSetFilecon(mgr, dev_source->data.nix.path, - imagelabel) < 0) + imagelabel, + true) < 0) goto done; } ret = 0; @@ -2438,13 +2462,14 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxSetFilecon(mgr, in, imagelabel) < 0) || - (virSecuritySELinuxSetFilecon(mgr, out, imagelabel) < 0)) { + if ((virSecuritySELinuxSetFilecon(mgr, in, imagelabel, true) < 0) || + (virSecuritySELinuxSetFilecon(mgr, out, imagelabel, true) < 0)) { goto done; } } else if (virSecuritySELinuxSetFilecon(mgr, dev_source->data.file.path, - imagelabel) < 0) { + imagelabel, + true) < 0) { goto done; } ret = 0; @@ -2492,7 +2517,7 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - false) < 0) + true) < 0) goto done; ret = 0; break; @@ -2501,7 +2526,7 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, if (!dev_source->data.nix.listen) { if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - false) < 0) + true) < 0) goto done; } ret = 0; @@ -2512,13 +2537,13 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxRestoreFileLabel(mgr, out, false) < 0) || - (virSecuritySELinuxRestoreFileLabel(mgr, in, false) < 0)) { + if ((virSecuritySELinuxRestoreFileLabel(mgr, out, true) < 0) || + (virSecuritySELinuxRestoreFileLabel(mgr, in, true) < 0)) { goto done; } } else if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - false) < 0) { + true) < 0) { goto done; } ret = 0; @@ -2570,7 +2595,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxRestoreFileLabel(mgr, database, false); + return virSecuritySELinuxRestoreFileLabel(mgr, database, true); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxRestoreChardevLabel(mgr, def, @@ -2665,23 +2690,23 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, rc = -1; if (def->os.loader && def->os.loader->nvram && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, true) < 0) rc = -1; if (def->os.kernel && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1; if (def->os.initrd && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true) < 0) rc = -1; if (def->os.dtb && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, true) < 0) rc = -1; if (def->os.slic_table && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, true) < 0) rc = -1; return rc; @@ -2726,7 +2751,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxSetFilecon(mgr, savefile, secdef->imagelabel); + return virSecuritySELinuxSetFilecon(mgr, savefile, secdef->imagelabel, true); } @@ -2741,7 +2766,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, savefile, false); + return virSecuritySELinuxRestoreFileLabel(mgr, savefile, true); } @@ -2984,7 +3009,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxSetFilecon(mgr, database, data->content_context); + return virSecuritySELinuxSetFilecon(mgr, database, data->content_context, true); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxSetChardevLabel(mgr, def, @@ -3075,32 +3100,32 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, if (def->os.loader && def->os.loader->nvram && secdef && secdef->imagelabel && virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram, - secdef->imagelabel) < 0) + secdef->imagelabel, true) < 0) return -1; if (def->os.kernel && virSecuritySELinuxSetFilecon(mgr, def->os.kernel, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (def->os.initrd && virSecuritySELinuxSetFilecon(mgr, def->os.initrd, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (def->os.dtb && virSecuritySELinuxSetFilecon(mgr, def->os.dtb, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (def->os.slic_table && virSecuritySELinuxSetFilecon(mgr, def->os.slic_table, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (stdin_path && virSecuritySELinuxSetFilecon(mgr, stdin_path, - data->content_context) < 0) + data->content_context, true) < 0) return -1; return 0; @@ -3259,7 +3284,7 @@ virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr, if (!seclabel || !seclabel->relabel) return 0; - return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel); + return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel, true); } @@ -3284,7 +3309,7 @@ virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr, char *filename = NULL; DIR *dir; - if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel))) + if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel, true))) return ret; if (!virFileIsDir(path)) @@ -3299,7 +3324,7 @@ virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr, break; } ret = virSecuritySELinuxSetFilecon(mgr, filename, - seclabel->imagelabel); + seclabel->imagelabel, true); VIR_FREE(filename); if (ret < 0) break; @@ -3333,7 +3358,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, char *filename = NULL; DIR *dir; - if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false))) + if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true))) return ret; if (!virFileIsDir(path)) @@ -3347,7 +3372,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, ret = -1; break; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, true); VIR_FREE(filename); if (ret < 0) break; -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:52AM +0200, Michal Privoznik wrote:
Just like previous commit allowed to enable or disable owner remembering for each individual path, do the same for SELinux driver. This is going to be needed in the next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 163 ++++++++++++++++++-------------- 1 file changed, 94 insertions(+), 69 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Some paths will not be restored. Because we can't possibly know if they are still in use or not. Reflect this in the test so that we can test more domains. Also see next commit for more detailed explanation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 44 ++++++++++++++++++++++++++++++++-------- tests/qemusecuritytest.c | 2 +- tests/qemusecuritytest.h | 2 +- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index e219a9f023..2a9095e1bf 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -353,20 +353,28 @@ int virFileUnlock(int fd ATTRIBUTE_UNUSED, } +typedef struct _checkOwnerData checkOwnerData; +struct _checkOwnerData { + const char **paths; + bool chown_fail; +}; + + static int checkOwner(void *payload, const void *name, - void *data) + void *opaque) { - bool *chown_fail = data; + checkOwnerData *data = opaque; uint32_t owner = *((uint32_t*) payload); - if (owner % 16 != DEFAULT_UID || - owner >> 16 != DEFAULT_GID) { + if ((owner % 16 != DEFAULT_UID || + owner >> 16 != DEFAULT_GID) && + !virStringListHasString(data->paths, name)) { fprintf(stderr, "Path %s wasn't restored back to its original owner\n", (const char *) name); - *chown_fail = true; + data->chown_fail = true; } return 0; @@ -391,22 +399,40 @@ printXATTR(void *payload, } -int checkPaths(void) +/** + * checkPaths: + * @paths: a NULL terminated list of paths expected not to be restored + * + * Check if all paths were restored and if no XATTR was left + * behind. Since restore is not done on all domain's paths, some + * paths are expected to be not restored. A list of such paths + * can be passed in @paths argument. If a path is not restored + * but it's on the list no error is indicated. + */ +int checkPaths(const char **paths) { int ret = -1; - bool chown_fail = false; + checkOwnerData data = { .paths = paths, .chown_fail = false }; bool xattr_fail = false; + size_t i; virMutexLock(&m); init_hash(); - if ((virHashForEach(chown_paths, checkOwner, &chown_fail)) < 0) + for (i = 0; paths && paths[i]; i++) { + if (!virHashLookup(chown_paths, paths[i])) { + fprintf(stderr, "Unexpected path restored: %s\n", paths[i]); + goto cleanup; + } + } + + if ((virHashForEach(chown_paths, checkOwner, &data)) < 0) goto cleanup; if ((virHashForEach(xattr_paths, printXATTR, &xattr_fail)) < 0) goto cleanup; - if (chown_fail || xattr_fail) + if (data.chown_fail || xattr_fail) goto cleanup; ret = 0; diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 86347f8625..65e08b4503 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -100,7 +100,7 @@ testDomain(const void *opaque) qemuSecurityRestoreAllLabel(data->driver, vm, false); - if (checkPaths() < 0) + if (checkPaths(NULL) < 0) goto cleanup; ret = 0; diff --git a/tests/qemusecuritytest.h b/tests/qemusecuritytest.h index 29c6a9c998..b76277a6d5 100644 --- a/tests/qemusecuritytest.h +++ b/tests/qemusecuritytest.h @@ -21,7 +21,7 @@ # define ENVVAR "LIBVIRT_QEMU_SECURITY_TEST" -extern int checkPaths(void); +extern int checkPaths(const char **paths); extern void freePaths(void); -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:53AM +0200, Michal Privoznik wrote:
Some paths will not be restored. Because we can't possibly know if they are still in use or not. Reflect this in the test so that we can test more domains. Also see next commit for more detailed explanation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemusecuritymock.c | 44 ++++++++++++++++++++++++++++++++-------- tests/qemusecuritytest.c | 2 +- tests/qemusecuritytest.h | 2 +- 3 files changed, 37 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This effectively reverts d7420430ce6 and adds new code. Here is the problem: Imagine a file X that is to be shared between two domains as a disk. Let the first domain (vm1) have seclabel remembering turned on and the other (vm2) has it turned off. Assume that both domains will run under the same user, but the original owner of X is different (i.e. trying to access X without relabelling leads to EPERM). Let's start vm1 first. This will cause X to be relabelled and to gain new attributes: trusted.libvirt.security.ref_dac="1" trusted.libvirt.security.dac="$originalOwner" When vm2 is started, X will again be relabelled, but since the new label is the same as X already has (because of vm1) nothing changes and vm1 and vm2 can access X just fine. Note that no XATTR is changed (especially the refcounter keeps its value of 1) because the vm2 domain has the feature turned off. Now, vm1 is shut off and vm2 continues running. In seclabel restore process we would get to X and since its refcounter is 1 we would restore the $originalOwner on it. But this is unsafe to do because vm2 is still using X (remember the assumption that $originalOwner and vm2's seclabel are distinct?). The problem is that refcounter stored in XATTRs doesn't reflect the actual times a resource is in use. Since I don't see any easy way around it let's just not store original owner on shared resources. Shared resource in world of domain disks is: - whole backing chain but the top layer, - read only disk (we don't require CDROM to be explicitly marked as shareable), - disk marked as shareable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 25 ++++++++++++++++++++++++- src/security/security_selinux.c | 27 +++++++++++++++++++++------ tests/qemusecuritytest.c | 24 +++++++++++++++++++++++- 3 files changed, 68 insertions(+), 8 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a39dae5226..56416e6f6a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -878,6 +878,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + bool remember; uid_t user; gid_t group; @@ -911,7 +912,21 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnership(mgr, src, NULL, user, group, true); + /* We can't do restore on shared resources safely. Not even + * with refcounting implemented in XATTRs because if there + * was a domain running with the feature turned off the + * refcounter in XATTRs would not reflect the actual number + * of times the resource is in use and thus the last restore + * on the resource (which actually restores the original + * owner) might cut off access to the domain with the feature + * disabled. + * For disks, a shared resource is the whole backing chain + * but the top layer, or read only image, or disk explicitly + * marked as shared. + */ + remember = src == parent && !src->readonly && !src->shared; + + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember); } @@ -948,6 +963,14 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; + /* Don't restore labels on readoly/shared disks, because other VMs may + * still be accessing these. Alternatively we could iterate over all + * running domains and try to figure out if it is in use, but this would + * not work for clustered filesystems, since we can't see running VMs using + * the file on other nodes. Safest bet is thus to skip the restore step. */ + if (src->readonly || src->shared) + return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (secdef && !secdef->relabel) return 0; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 3ac3b83e45..cb46004896 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1819,6 +1819,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; + bool remember; int ret; if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -1828,6 +1829,20 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0; + /* We can't do restore on shared resources safely. Not even + * with refcounting implemented in XATTRs because if there + * was a domain running with the feature turned off the + * refcounter in XATTRs would not reflect the actual number + * of times the resource is in use and thus the last restore + * on the resource (which actually restores the original + * owner) might cut off access to the domain with the feature + * disabled. + * For disks, a shared resource is the whole backing chain + * but the top layer, or read only image, or disk explicitly + * marked as shared. + */ + remember = src == parent && !src->readonly && !src->shared; + disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); if (parent) @@ -1839,29 +1854,29 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, return 0; ret = virSecuritySELinuxSetFilecon(mgr, src->path, - disk_seclabel->label, true); + disk_seclabel->label, remember); } else if (parent_seclabel && (!parent_seclabel->relabel || parent_seclabel->label)) { if (!parent_seclabel->relabel) return 0; ret = virSecuritySELinuxSetFilecon(mgr, src->path, - parent_seclabel->label, true); + parent_seclabel->label, remember); } else if (!parent || parent == src) { if (src->shared) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->file_context, - true); + remember); } else if (src->readonly) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->content_context, - true); + remember); } else if (secdef->imagelabel) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, secdef->imagelabel, - true); + remember); } else { ret = 0; } @@ -1869,7 +1884,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->content_context, - true); + remember); } if (ret == 1 && !disk_seclabel) { diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 65e08b4503..2d88979168 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -85,11 +85,32 @@ testDomain(const void *opaque) { const struct testData *data = opaque; VIR_AUTOUNREF(virDomainObjPtr) vm = NULL; + VIR_AUTOSTRINGLIST notRestored = NULL; + size_t i; int ret = -1; if (prepareObjects(data->driver, data->file, &vm) < 0) return -1; + for (i = 0; i < vm->def->ndisks; i++) { + virStorageSourcePtr src = vm->def->disks[i]->src; + virStorageSourcePtr n; + + if (!src) + continue; + + if (virStorageSourceIsLocalStorage(src) && src->path && + (src->shared || src->readonly) && + virStringListAdd(¬Restored, src->path) < 0) + return -1; + + for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) { + if (virStorageSourceIsLocalStorage(n) && n->path && + virStringListAdd(¬Restored, n->path) < 0) + return -1; + } + } + /* Mocking is enabled only when this env variable is set. * See mock code for explanation. */ if (setenv(ENVVAR, "1", 0) < 0) @@ -100,7 +121,7 @@ testDomain(const void *opaque) qemuSecurityRestoreAllLabel(data->driver, vm, false); - if (checkPaths(NULL) < 0) + if (checkPaths((const char **) notRestored) < 0) goto cleanup; ret = 0; @@ -144,6 +165,7 @@ mymain(void) DO_TEST_DOMAIN("console-virtio-unix"); DO_TEST_DOMAIN("controller-virtio-scsi"); DO_TEST_DOMAIN("disk-aio"); + DO_TEST_DOMAIN("disk-backing-chains-noindex"); DO_TEST_DOMAIN("disk-cache"); DO_TEST_DOMAIN("disk-cdrom"); DO_TEST_DOMAIN("disk-cdrom-bus-other"); -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:54AM +0200, Michal Privoznik wrote:
This effectively reverts d7420430ce6 and adds new code.
Here is the problem: Imagine a file X that is to be shared between two domains as a disk. Let the first domain (vm1) have seclabel remembering turned on and the other (vm2) has it turned off. Assume that both domains will run under the same user, but the original owner of X is different (i.e. trying to access X without relabelling leads to EPERM).
How do we get into this situation ? Is this the case when we have a guest which was running before libvirt was upgraded, and then a new guest is launched ?
Let's start vm1 first. This will cause X to be relabelled and to gain new attributes:
trusted.libvirt.security.ref_dac="1" trusted.libvirt.security.dac="$originalOwner"
When vm2 is started, X will again be relabelled, but since the new label is the same as X already has (because of vm1) nothing changes and vm1 and vm2 can access X just fine. Note that no XATTR is changed (especially the refcounter keeps its value of 1) because the vm2 domain has the feature turned off.
Now, vm1 is shut off and vm2 continues running. In seclabel restore process we would get to X and since its refcounter is 1 we would restore the $originalOwner on it. But this is unsafe to do because vm2 is still using X (remember the assumption that $originalOwner and vm2's seclabel are distinct?).
The problem is that refcounter stored in XATTRs doesn't reflect the actual times a resource is in use. Since I don't see any easy way around it let's just not store original owner on shared resources. Shared resource in world of domain disks is:
- whole backing chain but the top layer, - read only disk (we don't require CDROM to be explicitly marked as shareable), - disk marked as shareable.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 25 ++++++++++++++++++++++++- src/security/security_selinux.c | 27 +++++++++++++++++++++------ tests/qemusecuritytest.c | 24 +++++++++++++++++++++++- 3 files changed, 68 insertions(+), 8 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a39dae5226..56416e6f6a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -878,6 +878,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + bool remember; uid_t user; gid_t group;
@@ -911,7 +912,21 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; }
- return virSecurityDACSetOwnership(mgr, src, NULL, user, group, true); + /* We can't do restore on shared resources safely. Not even + * with refcounting implemented in XATTRs because if there + * was a domain running with the feature turned off the + * refcounter in XATTRs would not reflect the actual number + * of times the resource is in use and thus the last restore + * on the resource (which actually restores the original + * owner) might cut off access to the domain with the feature + * disabled. + * For disks, a shared resource is the whole backing chain + * but the top layer, or read only image, or disk explicitly + * marked as shared. + */ + remember = src == parent && !src->readonly && !src->shared; + + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember); }
@@ -948,6 +963,14 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0;
+ /* Don't restore labels on readoly/shared disks, because other VMs may + * still be accessing these. Alternatively we could iterate over all + * running domains and try to figure out if it is in use, but this would + * not work for clustered filesystems, since we can't see running VMs using + * the file on other nodes. Safest bet is thus to skip the restore step. */ + if (src->readonly || src->shared) + return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (secdef && !secdef->relabel) return 0; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 3ac3b83e45..cb46004896 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1819,6 +1819,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; + bool remember; int ret;
if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -1828,6 +1829,20 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0;
+ /* We can't do restore on shared resources safely. Not even + * with refcounting implemented in XATTRs because if there + * was a domain running with the feature turned off the + * refcounter in XATTRs would not reflect the actual number + * of times the resource is in use and thus the last restore + * on the resource (which actually restores the original + * owner) might cut off access to the domain with the feature + * disabled. + * For disks, a shared resource is the whole backing chain + * but the top layer, or read only image, or disk explicitly + * marked as shared. + */ + remember = src == parent && !src->readonly && !src->shared; + disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); if (parent) @@ -1839,29 +1854,29 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, return 0;
ret = virSecuritySELinuxSetFilecon(mgr, src->path, - disk_seclabel->label, true); + disk_seclabel->label, remember); } else if (parent_seclabel && (!parent_seclabel->relabel || parent_seclabel->label)) { if (!parent_seclabel->relabel) return 0;
ret = virSecuritySELinuxSetFilecon(mgr, src->path, - parent_seclabel->label, true); + parent_seclabel->label, remember); } else if (!parent || parent == src) { if (src->shared) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->file_context, - true); + remember); } else if (src->readonly) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->content_context, - true); + remember); } else if (secdef->imagelabel) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, secdef->imagelabel, - true); + remember); } else { ret = 0; } @@ -1869,7 +1884,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->content_context, - true); + remember); }
if (ret == 1 && !disk_seclabel) { diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 65e08b4503..2d88979168 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -85,11 +85,32 @@ testDomain(const void *opaque) { const struct testData *data = opaque; VIR_AUTOUNREF(virDomainObjPtr) vm = NULL; + VIR_AUTOSTRINGLIST notRestored = NULL; + size_t i; int ret = -1;
if (prepareObjects(data->driver, data->file, &vm) < 0) return -1;
+ for (i = 0; i < vm->def->ndisks; i++) { + virStorageSourcePtr src = vm->def->disks[i]->src; + virStorageSourcePtr n; + + if (!src) + continue; + + if (virStorageSourceIsLocalStorage(src) && src->path && + (src->shared || src->readonly) && + virStringListAdd(¬Restored, src->path) < 0) + return -1; + + for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) { + if (virStorageSourceIsLocalStorage(n) && n->path && + virStringListAdd(¬Restored, n->path) < 0) + return -1; + } + } + /* Mocking is enabled only when this env variable is set. * See mock code for explanation. */ if (setenv(ENVVAR, "1", 0) < 0) @@ -100,7 +121,7 @@ testDomain(const void *opaque)
qemuSecurityRestoreAllLabel(data->driver, vm, false);
- if (checkPaths(NULL) < 0) + if (checkPaths((const char **) notRestored) < 0) goto cleanup;
ret = 0; @@ -144,6 +165,7 @@ mymain(void) DO_TEST_DOMAIN("console-virtio-unix"); DO_TEST_DOMAIN("controller-virtio-scsi"); DO_TEST_DOMAIN("disk-aio"); + DO_TEST_DOMAIN("disk-backing-chains-noindex"); DO_TEST_DOMAIN("disk-cache"); DO_TEST_DOMAIN("disk-cdrom"); DO_TEST_DOMAIN("disk-cdrom-bus-other"); -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/17/19 3:29 PM, Daniel P. Berrangé wrote:
On Thu, Apr 25, 2019 at 10:19:54AM +0200, Michal Privoznik wrote:
This effectively reverts d7420430ce6 and adds new code.
Here is the problem: Imagine a file X that is to be shared between two domains as a disk. Let the first domain (vm1) have seclabel remembering turned on and the other (vm2) has it turned off. Assume that both domains will run under the same user, but the original owner of X is different (i.e. trying to access X without relabelling leads to EPERM).
How do we get into this situation ? Is this the case when we have a guest which was running before libvirt was upgraded, and then a new guest is launched ?
Yes, that's one of the possible scenarios. Another possible scenario would be (and this won't happen yet in reality beacuse NFS still does not implement XATTRs, but once they do we might hit it): two daemons and one shared NFS mount. One of the daemons has the feature enabled, the other has it disabled. But as I say, this won't happen with NFS today. But maybe there are some other shared filesystems which do implement XATTRs? Based on Wiki [1], OCFS2 does support it (even though I don't think there's anybody using it). 1: https://en.wikipedia.org/wiki/Extended_file_attributes Michal
On Thu, Jun 20, 2019 at 12:23:07PM +0200, Michal Privoznik wrote:
On 6/17/19 3:29 PM, Daniel P. Berrangé wrote:
On Thu, Apr 25, 2019 at 10:19:54AM +0200, Michal Privoznik wrote:
This effectively reverts d7420430ce6 and adds new code.
Here is the problem: Imagine a file X that is to be shared between two domains as a disk. Let the first domain (vm1) have seclabel remembering turned on and the other (vm2) has it turned off. Assume that both domains will run under the same user, but the original owner of X is different (i.e. trying to access X without relabelling leads to EPERM).
How do we get into this situation ? Is this the case when we have a guest which was running before libvirt was upgraded, and then a new guest is launched ?
Yes, that's one of the possible scenarios. Another possible scenario would be (and this won't happen yet in reality beacuse NFS still does not implement XATTRs, but once they do we might hit it): two daemons and one shared NFS mount. One of the daemons has the feature enabled, the other has it disabled. But as I say, this won't happen with NFS today. But maybe there are some other shared filesystems which do implement XATTRs?
IMHO if you are using a set of virt hosts with a shared NFS and have only enabled label mgmt on a subset of hosts that's a deployment error in general. Having said that, this is precisely the scearnio you'd have if you are part way through a libvirt upgrade across many hosts. Our core problem is that we have zero knowledge about other hosts, neither their config, or even whether they exist at all, so we can't do the safe thing automatically. I'm still not a fan of just giving up & deleting the feature though. How about we create a qemu.conf option to turn on/off label mgmt for shared volumes, defaulting to off. Document that it should only be turned off if all hosts are participating & no historical VMs are running ? In theory on upgrade, we could look at all running VMs on our own host and record the ref count data that is otherwise missing. That could at least let it work correctly on a the single-host non-shared FS scenario. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/20/19 1:58 PM, Daniel P. Berrangé wrote:
On Thu, Jun 20, 2019 at 12:23:07PM +0200, Michal Privoznik wrote:
On 6/17/19 3:29 PM, Daniel P. Berrangé wrote:
On Thu, Apr 25, 2019 at 10:19:54AM +0200, Michal Privoznik wrote:
This effectively reverts d7420430ce6 and adds new code.
Here is the problem: Imagine a file X that is to be shared between two domains as a disk. Let the first domain (vm1) have seclabel remembering turned on and the other (vm2) has it turned off. Assume that both domains will run under the same user, but the original owner of X is different (i.e. trying to access X without relabelling leads to EPERM).
How do we get into this situation ? Is this the case when we have a guest which was running before libvirt was upgraded, and then a new guest is launched ?
Yes, that's one of the possible scenarios. Another possible scenario would be (and this won't happen yet in reality beacuse NFS still does not implement XATTRs, but once they do we might hit it): two daemons and one shared NFS mount. One of the daemons has the feature enabled, the other has it disabled. But as I say, this won't happen with NFS today. But maybe there are some other shared filesystems which do implement XATTRs?
IMHO if you are using a set of virt hosts with a shared NFS and have only enabled label mgmt on a subset of hosts that's a deployment error in general.
Having said that, this is precisely the scearnio you'd have if you are part way through a libvirt upgrade across many hosts.
Our core problem is that we have zero knowledge about other hosts, neither their config, or even whether they exist at all, so we can't do the safe thing automatically.
Yep, this is the root cause of the problem.
I'm still not a fan of just giving up & deleting the feature though.
How about we create a qemu.conf option to turn on/off label mgmt for shared volumes, defaulting to off. Document that it should only be turned off if all hosts are participating & no historical VMs are running ?
This could work. But if possible, I'd probably do that in a follow up series? I mean, this series is long enough already and I can see it working/being merged as I write what you suggest.
In theory on upgrade, we could look at all running VMs on our own host and record the ref count data that is otherwise missing. That could at least let it work correctly on a the single-host non-shared FS scenario.
Yeah, since we don't know about other daemons we could do that only for local files. Michal
On Thu, Jun 20, 2019 at 02:32:40PM +0200, Michal Privoznik wrote:
On 6/20/19 1:58 PM, Daniel P. Berrangé wrote:
On Thu, Jun 20, 2019 at 12:23:07PM +0200, Michal Privoznik wrote:
On 6/17/19 3:29 PM, Daniel P. Berrangé wrote:
On Thu, Apr 25, 2019 at 10:19:54AM +0200, Michal Privoznik wrote:
This effectively reverts d7420430ce6 and adds new code.
Here is the problem: Imagine a file X that is to be shared between two domains as a disk. Let the first domain (vm1) have seclabel remembering turned on and the other (vm2) has it turned off. Assume that both domains will run under the same user, but the original owner of X is different (i.e. trying to access X without relabelling leads to EPERM).
How do we get into this situation ? Is this the case when we have a guest which was running before libvirt was upgraded, and then a new guest is launched ?
Yes, that's one of the possible scenarios. Another possible scenario would be (and this won't happen yet in reality beacuse NFS still does not implement XATTRs, but once they do we might hit it): two daemons and one shared NFS mount. One of the daemons has the feature enabled, the other has it disabled. But as I say, this won't happen with NFS today. But maybe there are some other shared filesystems which do implement XATTRs?
IMHO if you are using a set of virt hosts with a shared NFS and have only enabled label mgmt on a subset of hosts that's a deployment error in general.
Having said that, this is precisely the scearnio you'd have if you are part way through a libvirt upgrade across many hosts.
Our core problem is that we have zero knowledge about other hosts, neither their config, or even whether they exist at all, so we can't do the safe thing automatically.
Yep, this is the root cause of the problem.
I'm still not a fan of just giving up & deleting the feature though.
How about we create a qemu.conf option to turn on/off label mgmt for shared volumes, defaulting to off. Document that it should only be turned off if all hosts are participating & no historical VMs are running ?
This could work. But if possible, I'd probably do that in a follow up series? I mean, this series is long enough already and I can see it working/being merged as I write what you suggest.
Yep, no objection to that.
In theory on upgrade, we could look at all running VMs on our own host and record the ref count data that is otherwise missing. That could at least let it work correctly on a the single-host non-shared FS scenario.
Yeah, since we don't know about other daemons we could do that only for local files.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The purpose of this API is to allow caller move XATTRs (or remove them) from one file to another. This will be needed when moving top level of disk chain (either by introducing new HEAD or removing it). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 39 +++++++++++++++++++++++++++++++++ src/security/security_manager.h | 4 ++++ src/security/security_nop.c | 10 +++++++++ src/security/security_stack.c | 20 +++++++++++++++++ 6 files changed, 79 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5368392882..670daae5a2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1388,6 +1388,7 @@ virSecurityManagerGetModel; virSecurityManagerGetMountOptions; virSecurityManagerGetNested; virSecurityManagerGetProcessLabel; +virSecurityManagerMoveImageMetadata; virSecurityManagerNew; virSecurityManagerNewDAC; virSecurityManagerNewStack; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 36cf9da037..998fe9697c 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -120,6 +120,10 @@ typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virStorageSourcePtr src, virSecurityDomainImageLabelFlags flags); +typedef int (*virSecurityDomainMoveImageMetadata) (virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst); typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem); @@ -170,6 +174,7 @@ struct _virSecurityDriver { virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; + virSecurityDomainMoveImageMetadata domainMoveImageMetadata; virSecurityDomainSetMemoryLabel domainSetSecurityMemoryLabel; virSecurityDomainRestoreMemoryLabel domainRestoreSecurityMemoryLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 74ab0d0dd3..c205c3bf17 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -432,6 +432,45 @@ virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, } +/** + * virSecurityManagerMoveImageMetadata: + * @mgr: security manager + * @pid: domain's PID + * @src: source of metadata + * @dst: destination to move metadata to + * + * For given source @src, metadata is moved to destination @dst. + * + * If @dst is NULL then metadata is removed from @src and not + * stored anywhere. + * + * If @pid is not -1 enther the @pid mount namespace (usually + * @pid refers to a domain) and perform the move from there. If + * @pid is -1 then the move is performed from the caller's + * namespace. + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + if (mgr->drv->domainMoveImageMetadata) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainMoveImageMetadata(mgr, pid, src, dst); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 7e174a33ee..33e79b2095 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -160,6 +160,10 @@ int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, virStorageSourcePtr src, virSecurityDomainImageLabelFlags flags); +int virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst); int virSecurityManagerSetMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 9b3263ad77..966b9d41a1 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -224,6 +224,15 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +static int +virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid ATTRIBUTE_UNUSED, + virStorageSourcePtr src ATTRIBUTE_UNUSED, + virStorageSourcePtr dst ATTRIBUTE_UNUSED) +{ + return 0; +} + static int virSecurityDomainSetMemoryLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED, @@ -280,6 +289,7 @@ virSecurityDriver virSecurityDriverNop = { .domainSetSecurityImageLabel = virSecurityDomainSetImageLabelNop, .domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop, + .domainMoveImageMetadata = virSecurityDomainMoveImageMetadataNop, .domainSetSecurityMemoryLabel = virSecurityDomainSetMemoryLabelNop, .domainRestoreSecurityMemoryLabel = virSecurityDomainRestoreMemoryLabelNop, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index eba918e257..d445c0773e 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -599,6 +599,25 @@ virSecurityStackRestoreImageLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerMoveImageMetadata(item->securityManager, + pid, src, dst) < 0) + rc = -1; + } + + return rc; +} + static int virSecurityStackSetMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -785,6 +804,7 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityImageLabel = virSecurityStackSetImageLabel, .domainRestoreSecurityImageLabel = virSecurityStackRestoreImageLabel, + .domainMoveImageMetadata = virSecurityStackMoveImageMetadata, .domainSetSecurityMemoryLabel = virSecurityStackSetMemoryLabel, .domainRestoreSecurityMemoryLabel = virSecurityStackRestoreMemoryLabel, -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:55AM +0200, Michal Privoznik wrote:
The purpose of this API is to allow caller move XATTRs (or remove them) from one file to another. This will be needed when moving top level of disk chain (either by introducing new HEAD or removing it).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 39 +++++++++++++++++++++++++++++++++ src/security/security_manager.h | 4 ++++ src/security/security_nop.c | 10 +++++++++ src/security/security_stack.c | 20 +++++++++++++++++ 6 files changed, 79 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
A simple helper function that would be used from DAC and SELinux drivers. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_util.c | 63 ++++++++++++++++++++++++++++++++++++ src/security/security_util.h | 5 +++ 2 files changed, 68 insertions(+) diff --git a/src/security/security_util.c b/src/security/security_util.c index 3c24d7cded..ad265b0bc5 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -256,3 +256,66 @@ virSecuritySetRememberedLabel(const char *name, VIR_FREE(ref_name); return ret; } + + +int +virSecurityMoveRememberedLabel(const char *name, + const char *src, + const char *dst) +{ + VIR_AUTOFREE(char *) ref_name = NULL; + VIR_AUTOFREE(char *) ref_value = NULL; + VIR_AUTOFREE(char *) attr_name = NULL; + VIR_AUTOFREE(char *) attr_value = NULL; + + if (!(ref_name = virSecurityGetRefCountAttrName(name)) | + !(attr_name = virSecurityGetAttrName(name))) + return -1; + + if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) { + return -2; + } else if (errno != ENODATA) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + ref_name, src); + return -1; + } + } + + if (virFileGetXAttrQuiet(src, attr_name, &attr_value) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) { + return -2; + } else if (errno != ENODATA) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + attr_name, src); + return -1; + } + } + + if (ref_value && + virFileRemoveXAttr(src, ref_name) < 0) { + return -1; + } + + if (attr_value && + virFileRemoveXAttr(src, attr_name) < 0) { + return -1; + } + + if (dst) { + if (ref_value && + virFileSetXAttr(dst, ref_name, ref_value) < 0) { + return -1; + } + + if (attr_value && + virFileSetXAttr(dst, attr_name, attr_value) < 0) { + ignore_value(virFileRemoveXAttr(dst, ref_name)); + return -1; + } + } + + return 0; +} diff --git a/src/security/security_util.h b/src/security/security_util.h index bc977ed65d..f727e2e3e5 100644 --- a/src/security/security_util.h +++ b/src/security/security_util.h @@ -29,4 +29,9 @@ virSecuritySetRememberedLabel(const char *name, const char *path, const char *label); +int +virSecurityMoveRememberedLabel(const char *name, + const char *src, + const char *dst); + #endif /* LIBVIRT_SECURITY_UTIL_H */ -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:56AM +0200, Michal Privoznik wrote:
A simple helper function that would be used from DAC and SELinux drivers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_util.c | 63 ++++++++++++++++++++++++++++++++++++ src/security/security_util.h | 5 +++ 2 files changed, 68 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 62 +++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 56416e6f6a..137daf5d28 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1015,6 +1015,67 @@ virSecurityDACRestoreImageLabel(virSecurityManagerPtr mgr, } +struct virSecurityDACMoveImageMetadataData { + virSecurityManagerPtr mgr; + const char *src; + const char *dst; +}; + + +static int +virSecurityDACMoveImageMetadataHelper(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virSecurityDACMoveImageMetadataData *data = opaque; + const char *paths[2] = { data->src, data->dst }; + virSecurityManagerMetadataLockStatePtr state; + int ret; + + if (!(state = virSecurityManagerMetadataLock(data->mgr, paths, ARRAY_CARDINALITY(paths)))) + return -1; + + ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src, data->dst); + virSecurityManagerMetadataUnlock(data->mgr, &state); + return ret; +} + + +static int +virSecurityDACMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + struct virSecurityDACMoveImageMetadataData data = { .mgr = mgr, 0 }; + int rc; + + /* If dynamicOwnership is turned off, or owner remembering is + * not enabled there's nothing for us to do. */ + if (!priv->dynamicOwnership) + return 0; + + if (src && virStorageSourceIsLocalStorage(src)) + data.src = src->path; + + if (dst && virStorageSourceIsLocalStorage(dst)) + data.dst = dst->path; + + if (!data.src) + return 0; + + if (pid == -1) { + rc = virProcessRunInFork(virSecurityDACMoveImageMetadataHelper, &data); + } else { + rc = virProcessRunInMountNamespace(pid, + virSecurityDACMoveImageMetadataHelper, + &data); + } + + return rc; +} + + static int virSecurityDACSetHostdevLabelHelper(const char *file, void *opaque) @@ -2384,6 +2445,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityImageLabel = virSecurityDACSetImageLabel, .domainRestoreSecurityImageLabel = virSecurityDACRestoreImageLabel, + .domainMoveImageMetadata = virSecurityDACMoveImageMetadata, .domainSetSecurityMemoryLabel = virSecurityDACSetMemoryLabel, .domainRestoreSecurityMemoryLabel = virSecurityDACRestoreMemoryLabel, -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:57AM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_dac.c | 62 +++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index cb46004896..ea20373a90 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1926,6 +1926,62 @@ virSecuritySELinuxSetImageLabel(virSecurityManagerPtr mgr, } +struct virSecuritySELinuxMoveImageMetadataData { + virSecurityManagerPtr mgr; + const char *src; + const char *dst; +}; + + +static int +virSecuritySELinuxMoveImageMetadataHelper(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virSecuritySELinuxMoveImageMetadataData *data = opaque; + const char *paths[2] = { data->src, data->dst }; + virSecurityManagerMetadataLockStatePtr state; + int ret; + + if (!(state = virSecurityManagerMetadataLock(data->mgr, paths, ARRAY_CARDINALITY(paths)))) + return -1; + + ret = virSecurityMoveRememberedLabel(SECURITY_SELINUX_NAME, data->src, data->dst); + virSecurityManagerMetadataUnlock(data->mgr, &state); + return ret; +} + + +static int +virSecuritySELinuxMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + struct virSecuritySELinuxMoveImageMetadataData data = { .mgr = mgr, 0 }; + int rc; + + if (src && virStorageSourceIsLocalStorage(src)) + data.src = src->path; + + if (dst && virStorageSourceIsLocalStorage(dst)) + data.dst = dst->path; + + if (!data.src) + return 0; + + if (pid == -1) { + rc = virProcessRunInFork(virSecuritySELinuxMoveImageMetadataHelper, + &data); + } else { + rc = virProcessRunInMountNamespace(pid, + virSecuritySELinuxMoveImageMetadataHelper, + &data); + } + + return rc; +} + + static int virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque) { @@ -3475,6 +3531,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityImageLabel = virSecuritySELinuxSetImageLabel, .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreImageLabel, + .domainMoveImageMetadata = virSecuritySELinuxMoveImageMetadata, .domainSetSecurityMemoryLabel = virSecuritySELinuxSetMemoryLabel, .domainRestoreSecurityMemoryLabel = virSecuritySELinuxRestoreMemoryLabel, -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:58AM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_security.c | 19 +++++++++++++++++++ src/qemu/qemu_security.h | 5 +++++ 2 files changed, 24 insertions(+) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 229581a757..87209d3781 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -162,6 +162,25 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, } +int +qemuSecurityMoveImageMetadata(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + pid_t pid = -1; + + if (!priv->rememberOwner) + return 0; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + return virSecurityManagerMoveImageMetadata(driver->securityManager, pid, src, dst); +} + + int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 546a66f284..c62724ed05 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -44,6 +44,11 @@ int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virStorageSourcePtr src, bool backingChain); +int qemuSecurityMoveImageMetadata(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + virStorageSourcePtr dst); + int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev); -- 2.21.0
On Thu, Apr 25, 2019 at 10:19:59AM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_security.c | 19 +++++++++++++++++++ src/qemu/qemu_security.h | 5 +++++ 2 files changed, 24 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_blockjob.c | 6 ++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index fa7e4c8625..1b4e30ba01 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -37,6 +37,7 @@ #include "locking/domain_lock.h" #include "viralloc.h" #include "virstring.h" +#include "qemu_security.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -275,6 +276,11 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, * want to only revoke the non-shared portion of the chain); so for * now, we leak the access to the original. */ virDomainLockImageDetach(driver->lockManager, vm, disk->src); + + /* Move secret driver metadata */ + if (qemuSecurityMoveImageMetadata(driver, vm, disk->src, disk->mirror) < 0) + VIR_WARN("Unable to move disk metadata on vm %s", vm->def->name); + virObjectUnref(disk->src); disk->src = disk->mirror; } else { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31d8647eee..82770b49ad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15159,22 +15159,33 @@ qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src) /** * qemuDomainSnapshotUpdateDiskSources: + * @driver: QEMU driver + * @vm: domain object * @dd: snapshot disk data object * @persist: set to true if persistent config of the VM was changed * * Updates disk definition after a successful snapshot. */ static void -qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd, +qemuDomainSnapshotUpdateDiskSources(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainSnapshotDiskDataPtr dd, bool *persist) { - if (!dd->src) + if (!dd->src) { + /* Remove old metadata */ + if (qemuSecurityMoveImageMetadata(driver, vm, dd->disk->src, NULL) < 0) + VIR_WARN("Unable to remove disk metadata on vm %s", vm->def->name); return; + } /* storage driver access won'd be needed */ if (dd->initialized) virStorageFileDeinit(dd->src); + if (qemuSecurityMoveImageMetadata(driver, vm, dd->disk->src, dd->src) < 0) + VIR_WARN("Unable to move disk metadata on vm %s", vm->def->name); + /* the old disk image is now readonly */ dd->disk->src->readonly = true; @@ -15299,7 +15310,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", ret >= 0); if (ret == 0) - qemuDomainSnapshotUpdateDiskSources(dd, &persist); + qemuDomainSnapshotUpdateDiskSources(driver, vm, dd, &persist); } if (ret < 0) -- 2.21.0
On Thu, Apr 25, 2019 at 10:20:00AM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_blockjob.c | 6 ++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This reverts commit fc3990c7e64be1da1631952d3ec384ebef50e125. Now that all the reported bugs are fixed let's turn the feature back on. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- docs/news.xml | 13 +++++++++++++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 +++++ src/qemu/qemu_conf.c | 4 ++++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 24 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index e0cab23c49..95b78942ce 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -25,6 +25,19 @@ <section title="New features"> </section> <section title="Improvements"> + <change> + <summary> + Remember original owners and SELinux labels of files + </summary> + <description> + When a domain is starting up libvirt changes DAC and + SELinux labels so that domain can access it. However, + it never remembered the original labels and therefore + the file was returned back to <code>root:root</code>. + With this release, the original labels are remembered + and restored properly. + </description> + </change> </section> <section title="Bug fixes"> </section> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index b311f02da6..868f7b313c 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -72,6 +72,7 @@ module Libvirtd_qemu = | str_entry "user" | str_entry "group" | bool_entry "dynamic_ownership" + | bool_entry "remember_owner" | str_array_entry "cgroup_controllers" | str_array_entry "cgroup_device_acl" | int_entry "seccomp_sandbox" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 334b4cd4ee..12357461c4 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -456,6 +456,11 @@ # Set to 0 to disable file ownership changes. #dynamic_ownership = 1 +# Whether libvirt should remember and restore the original +# ownership over files it is relabeling. Defaults to 1, set +# to 0 to disable the feature. +#remember_owner = 1 + # What cgroup controllers to make use of with QEMU guests # # - 'cpu' - use for scheduler tunables diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index daea11dacb..b418d33c61 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -145,6 +145,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->group = (gid_t)-1; } cfg->dynamicOwnership = privileged; + cfg->rememberOwner = privileged; cfg->cgroupControllers = -1; /* -1 == auto-detect */ @@ -908,6 +909,9 @@ virQEMUDriverConfigLoadSecurityEntry(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "dynamic_ownership", &cfg->dynamicOwnership) < 0) return -1; + if (virConfGetValueBool(conf, "remember_owner", &cfg->rememberOwner) < 0) + return -1; + if (virConfGetValueStringList(conf, "cgroup_controllers", false, &controllers) < 0) return -1; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index fea1d308b7..f95496ce4d 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -44,6 +44,7 @@ module Test_libvirtd_qemu = { "user" = "root" } { "group" = "root" } { "dynamic_ownership" = "1" } +{ "remember_owner" = "1" } { "cgroup_controllers" { "1" = "cpu" } { "2" = "devices" } -- 2.21.0
On Thu, Apr 25, 2019 at 10:20:01AM +0200, Michal Privoznik wrote:
This reverts commit fc3990c7e64be1da1631952d3ec384ebef50e125.
Now that all the reported bugs are fixed let's turn the feature back on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Cole Robinson <crobinso@redhat.com> --- docs/news.xml | 13 +++++++++++++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 +++++ src/qemu/qemu_conf.c | 4 ++++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 24 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 4/25/19 10:19 AM, Michal Privoznik wrote:
This is meant for next release to have the most time possible for testing. Some of the patches were ACKed in v3 already but since they don't make sense on their own I haven't pushed them.
v4 of:
https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html
As usual, you can find (not only these) patches on my github:
https://github.com/zippy2/libvirt branch xattr_fixes_v4
Ping? Michal
On Mon, Jun 03, 2019 at 06:07:02PM +0200, Michal Privoznik wrote:
On 4/25/19 10:19 AM, Michal Privoznik wrote:
This is meant for next release to have the most time possible for testing. Some of the patches were ACKed in v3 already but since they don't make sense on their own I haven't pushed them.
v4 of:
https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html
As usual, you can find (not only these) patches on my github:
https://github.com/zippy2/libvirt branch xattr_fixes_v4
Ping?
Looks good - just have 1 question against patch 18 before I can ack that patch. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/17/19 3:34 PM, Daniel P. Berrangé wrote:
On Mon, Jun 03, 2019 at 06:07:02PM +0200, Michal Privoznik wrote:
On 4/25/19 10:19 AM, Michal Privoznik wrote:
This is meant for next release to have the most time possible for testing. Some of the patches were ACKed in v3 already but since they don't make sense on their own I haven't pushed them.
v4 of:
https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html
As usual, you can find (not only these) patches on my github:
https://github.com/zippy2/libvirt branch xattr_fixes_v4
Ping?
Looks good - just have 1 question against patch 18 before I can ack that patch.
Thank you both Dan and Cole for the review! However, given how close to the freeze we are and how intrusive this change is, I'd rather keep these in a local branch and merge only after the release to give us the longest window possible for test. Michal
On 6/20/19 2:39 PM, Michal Privoznik wrote:
On 6/17/19 3:34 PM, Daniel P. Berrangé wrote:
On Mon, Jun 03, 2019 at 06:07:02PM +0200, Michal Privoznik wrote:
On 4/25/19 10:19 AM, Michal Privoznik wrote:
This is meant for next release to have the most time possible for testing. Some of the patches were ACKed in v3 already but since they don't make sense on their own I haven't pushed them.
v4 of:
https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html
As usual, you can find (not only these) patches on my github:
https://github.com/zippy2/libvirt branch xattr_fixes_v4
Ping?
Looks good - just have 1 question against patch 18 before I can ack that patch.
Thank you both Dan and Cole for the review! However, given how close to the freeze we are and how intrusive this change is, I'd rather keep these in a local branch and merge only after the release to give us the longest window possible for test.
This is now pushed. I'll work on the follow up patches soon. Michal
On Wed, Jul 03, 2019 at 08:56:29AM +0200, Michal Prívozník wrote:
On 6/20/19 2:39 PM, Michal Privoznik wrote:
On 6/17/19 3:34 PM, Daniel P. Berrangé wrote:
On Mon, Jun 03, 2019 at 06:07:02PM +0200, Michal Privoznik wrote:
On 4/25/19 10:19 AM, Michal Privoznik wrote:
This is meant for next release to have the most time possible for testing. Some of the patches were ACKed in v3 already but since they don't make sense on their own I haven't pushed them.
v4 of:
https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html
As usual, you can find (not only these) patches on my github:
https://github.com/zippy2/libvirt branch xattr_fixes_v4
Ping?
Looks good - just have 1 question against patch 18 before I can ack that patch.
Thank you both Dan and Cole for the review! However, given how close to the freeze we are and how intrusive this change is, I'd rather keep these in a local branch and merge only after the release to give us the longest window possible for test.
This is now pushed. I'll work on the follow up patches soon.
Looks like we hit a unit test failure on the FreeBSD CI systems. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 7/3/19 1:17 PM, Daniel P. Berrangé wrote:
On Wed, Jul 03, 2019 at 08:56:29AM +0200, Michal Prívozník wrote:
On 6/20/19 2:39 PM, Michal Privoznik wrote:
On 6/17/19 3:34 PM, Daniel P. Berrangé wrote:
On Mon, Jun 03, 2019 at 06:07:02PM +0200, Michal Privoznik wrote:
On 4/25/19 10:19 AM, Michal Privoznik wrote:
This is meant for next release to have the most time possible for testing. Some of the patches were ACKed in v3 already but since they don't make sense on their own I haven't pushed them.
v4 of:
https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html
As usual, you can find (not only these) patches on my github:
https://github.com/zippy2/libvirt branch xattr_fixes_v4
Ping?
Looks good - just have 1 question against patch 18 before I can ack that patch.
Thank you both Dan and Cole for the review! However, given how close to the freeze we are and how intrusive this change is, I'd rather keep these in a local branch and merge only after the release to give us the longest window possible for test.
This is now pushed. I'll work on the follow up patches soon.
Looks like we hit a unit test failure on the FreeBSD CI systems.
Interestingly, I'm unable to reproduce on my virtual FreeBSD. From the CI logs it looks like the test did not pick up XATTR functions from qemusecuritymock but the real ones from util/virfile.c. But then again, something fishy must be going on since I cannot reproduce this on my FreeBSD machine. Michal
On Thu, Jul 04, 2019 at 08:22:04AM +0200, Michal Prívozník wrote:
On 7/3/19 1:17 PM, Daniel P. Berrangé wrote:
On Wed, Jul 03, 2019 at 08:56:29AM +0200, Michal Prívozník wrote:
On 6/20/19 2:39 PM, Michal Privoznik wrote:
On 6/17/19 3:34 PM, Daniel P. Berrangé wrote:
On Mon, Jun 03, 2019 at 06:07:02PM +0200, Michal Privoznik wrote:
On 4/25/19 10:19 AM, Michal Privoznik wrote: > > This is meant for next release to have the most time possible for > testing. Some of the patches were ACKed in v3 already but since they > don't make sense on their own I haven't pushed them. > > v4 of: > > https://www.redhat.com/archives/libvir-list/2019-March/msg01948.html > > As usual, you can find (not only these) patches on my github: > > https://github.com/zippy2/libvirt branch xattr_fixes_v4
Ping?
Looks good - just have 1 question against patch 18 before I can ack that patch.
Thank you both Dan and Cole for the review! However, given how close to the freeze we are and how intrusive this change is, I'd rather keep these in a local branch and merge only after the release to give us the longest window possible for test.
This is now pushed. I'll work on the follow up patches soon.
Looks like we hit a unit test failure on the FreeBSD CI systems.
Interestingly, I'm unable to reproduce on my virtual FreeBSD. From the CI logs it looks like the test did not pick up XATTR functions from qemusecuritymock but the real ones from util/virfile.c. But then again, something fishy must be going on since I cannot reproduce this on my FreeBSD machine.
I could reproduce it .... once I actually installed the prereqs to make QEMU get enabled in configure - for some reason we have a dep on readline in the m4/virt-yajl.m4 file that looks liek its probobly a bug. Anyway it turned out to be a wierd compiler optimization problem but was easily worked around by just mocking one more symbol, so I didn't bother to try to understand the root cause further. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, 2019-07-05 at 18:25 +0100, Daniel P. Berrangé wrote:
On Thu, Jul 04, 2019 at 08:22:04AM +0200, Michal Prívozník wrote:
Interestingly, I'm unable to reproduce on my virtual FreeBSD. From the CI logs it looks like the test did not pick up XATTR functions from qemusecuritymock but the real ones from util/virfile.c. But then again, something fishy must be going on since I cannot reproduce this on my FreeBSD machine.
I could reproduce it .... once I actually installed the prereqs to make QEMU get enabled in configure
I guess you guys didn't use lcitool to prepare the FreeBSD guest in question? Or is there some problem with it?
for some reason we have a dep on readline in the m4/virt-yajl.m4 file that looks liek its probobly a bug.
Yeah, I told Jano to look at m4/virt-readline.m4 for inspiration but perhaps he took it a bit too far ;) And then of course I failed to notice during review :( I just posted a patch fixing this oversight: https://www.redhat.com/archives/libvir-list/2019-July/msg00280.html -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Jul 08, 2019 at 10:00:47AM +0200, Andrea Bolognani wrote:
On Fri, 2019-07-05 at 18:25 +0100, Daniel P. Berrangé wrote:
On Thu, Jul 04, 2019 at 08:22:04AM +0200, Michal Prívozník wrote:
Interestingly, I'm unable to reproduce on my virtual FreeBSD. From the CI logs it looks like the test did not pick up XATTR functions from qemusecuritymock but the real ones from util/virfile.c. But then again, something fishy must be going on since I cannot reproduce this on my FreeBSD machine.
I could reproduce it .... once I actually installed the prereqs to make QEMU get enabled in configure
I guess you guys didn't use lcitool to prepare the FreeBSD guest in question? Or is there some problem with it?
This is a general purpose FreeBSD I already have setup for other purposes. When I found it didn't reproduce the failure, then I did actually setup a FreeBSD12 guest with lcitool. BTW, lcitool fails when you don't have any $HOME/.ssh/id_rsa.pub file, which is always for me, as my guests run on a remote server and my key is on my laptop. It ought to get the key by using "ssh-add -L" to get it from the agent if no local key exists. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2019-07-09 at 10:07 +0100, Daniel P. Berrangé wrote:
BTW, lcitool fails when you don't have any $HOME/.ssh/id_rsa.pub file, which is always for me, as my guests run on a remote server and my key is on my laptop.
It ought to get the key by using "ssh-add -L" to get it from the agent if no local key exists.
Yeah, this is an issue that I've known about for a long time but unfortunately never had the time to address :( -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník