[libvirt] [PATCH 0/3] Fix domain restore problems when selinux is enforcing
Prior to this patch series, restoring a domain with selinux set to enforcing would fail, because the function that sets the label on the file to allow qemu to read it did not have the name of the file (see the comments in the individual patches). A patch from Jamie Stranboge (2b57478ef0a0a983cc6a47b98300c8359f9708d0) added the filename to the args passed down into the security driver; the first patch of this series takes advantage of that to properly set the label. Patches 2 and 3 solve a problem with restoring a domain from an NFS share - in this case the selinux functions will fail (as will functions trying to set the uid of the file, if it is a root-squashed share). The solution to this is just ignore the failure. qemudDomainSaveFlag previously had a bit of code that detected if a particular path was on an NFS share; this code was moved into a utility function so it could be re-used during domain restore - if the security driver fails to set the label, and the file is on an NFS share, we ignore the failure, otherwise we behave as before.
Since vm->def->os.kernel (the normal place the path is found) is null during a domain restore, use the stdin_path that is passed into SELinuxSetSecurityAllLabel instead. Also restore the label to its original value after qemu is finished with the file. Prior to this patch, qemu domain restore did not function properly if selinux was set to enforce. --- src/qemu/qemu_driver.c | 6 +++++- src/security/security_selinux.c | 12 ++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f4e082..9140b50 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6208,7 +6208,6 @@ error: return -1; } -/* TODO: check seclabel restore */ static int ATTRIBUTE_NONNULL(6) qemudDomainSaveImageStartVM(virConnectPtr conn, struct qemud_driver *driver, @@ -6320,6 +6319,11 @@ qemudDomainSaveImageStartVM(virConnectPtr conn, ret = 0; out: + if (driver->securityDriver && + driver->securityDriver->domainRestoreSavedStateLabel && + driver->securityDriver->domainRestoreSavedStateLabel(vm, path) == -1) + VIR_WARN("failed to restore save state label on %s", path); + return ret; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2b43f2d..7500f1d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -859,7 +859,7 @@ SELinuxClearSecuritySocketLabel(virSecurityDriverPtr drv, } static int -SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_UNUSED) +SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int i; @@ -882,9 +882,13 @@ SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_ return -1; } - if (vm->def->os.kernel && - SELinuxSetFilecon(vm->def->os.kernel, default_content_context) < 0) - return -1; + if (vm->def->os.kernel) { + if (SELinuxSetFilecon(vm->def->os.kernel, default_content_context) < 0) + return -1; + } else if (stdin_path) { + if (SELinuxSetFilecon(stdin_path, default_content_context) < 0) + return -1; + } if (vm->def->os.initrd && SELinuxSetFilecon(vm->def->os.initrd, default_content_context) < 0) -- 1.7.1
On Fri, Jun 25, 2010 at 07:42:13AM -0400, Laine Stump wrote:
Since vm->def->os.kernel (the normal place the path is found) is null during a domain restore, use the stdin_path that is passed into SELinuxSetSecurityAllLabel instead.
Also restore the label to its original value after qemu is finished with the file.
Prior to this patch, qemu domain restore did not function properly if selinux was set to enforce. --- src/qemu/qemu_driver.c | 6 +++++- src/security/security_selinux.c | 12 ++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f4e082..9140b50 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6208,7 +6208,6 @@ error: return -1; }
-/* TODO: check seclabel restore */ static int ATTRIBUTE_NONNULL(6) qemudDomainSaveImageStartVM(virConnectPtr conn, struct qemud_driver *driver, @@ -6320,6 +6319,11 @@ qemudDomainSaveImageStartVM(virConnectPtr conn, ret = 0;
out: + if (driver->securityDriver && + driver->securityDriver->domainRestoreSavedStateLabel && + driver->securityDriver->domainRestoreSavedStateLabel(vm, path) == -1) + VIR_WARN("failed to restore save state label on %s", path); + return ret; }
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2b43f2d..7500f1d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -859,7 +859,7 @@ SELinuxClearSecuritySocketLabel(virSecurityDriverPtr drv, }
static int -SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_UNUSED) +SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int i; @@ -882,9 +882,13 @@ SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_ return -1; }
- if (vm->def->os.kernel && - SELinuxSetFilecon(vm->def->os.kernel, default_content_context) < 0) - return -1; + if (vm->def->os.kernel) { + if (SELinuxSetFilecon(vm->def->os.kernel, default_content_context) < 0) + return -1; + } else if (stdin_path) { + if (SELinuxSetFilecon(stdin_path, default_content_context) < 0) + return -1; + }
This doesn't make sense to me. Labelling of the kernel and labeling of stdin_path are completely separate tasks, so shouldn't be in an if/elseif arrangement. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 06/25/2010 07:57 AM, Daniel P. Berrange wrote:
On Fri, Jun 25, 2010 at 07:42:13AM -0400, Laine Stump wrote:
- if (vm->def->os.kernel&& - SELinuxSetFilecon(vm->def->os.kernel, default_content_context)< 0) - return -1; + if (vm->def->os.kernel) { + if (SELinuxSetFilecon(vm->def->os.kernel, default_content_context)< 0) + return -1; + } else if (stdin_path) { + if (SELinuxSetFilecon(stdin_path, default_content_context)< 0) + return -1; + }
This doesn't make sense to me. Labelling of the kernel and labeling of stdin_path are completely separate tasks, so shouldn't be in an if/elseif arrangement.
Heh. The name didn't really make sense to me either, but my slight misunderstanding of the scope of the problem made me think that in some cases the filename would be in vm->def, and in others not, and that seemed the only place already being used. Now that I've looked back over the code, I see that this function is only called in one place, and the filename is *never* available in vm->def; it all makes much more sense now. I'm preparing a v2. The proper thing is to just add: if (stdin_path && SELinuxSetFilecon(stdin_path, default_content_context) < 0)) return -1; correct? Or is there a different context that would be better suited? (default content_context certainly works).
On Fri, Jun 25, 2010 at 09:41:36AM -0400, Laine Stump wrote:
On 06/25/2010 07:57 AM, Daniel P. Berrange wrote:
On Fri, Jun 25, 2010 at 07:42:13AM -0400, Laine Stump wrote:
- if (vm->def->os.kernel&& - SELinuxSetFilecon(vm->def->os.kernel, default_content_context)< 0) - return -1; + if (vm->def->os.kernel) { + if (SELinuxSetFilecon(vm->def->os.kernel, default_content_context)< 0) + return -1; + } else if (stdin_path) { + if (SELinuxSetFilecon(stdin_path, default_content_context)< 0) + return -1; + }
This doesn't make sense to me. Labelling of the kernel and labeling of stdin_path are completely separate tasks, so shouldn't be in an if/elseif arrangement.
Heh. The name didn't really make sense to me either, but my slight misunderstanding of the scope of the problem made me think that in some cases the filename would be in vm->def, and in others not, and that seemed the only place already being used.
Now that I've looked back over the code, I see that this function is only called in one place, and the filename is *never* available in vm->def; it all makes much more sense now.
I'm preparing a v2. The proper thing is to just add:
if (stdin_path && SELinuxSetFilecon(stdin_path, default_content_context) < 0)) return -1;
correct? Or is there a different context that would be better suited? (default content_context certainly works).
This is good. QEMU only needs to be able to read from the file, never write to it during restore, so default_content_context is the right one. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This code was previously used inline during qemudDomainSaveFlag. This patch moves it into a utility function in preparation for it to be used elsewhere. --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 83 +++++++++++++--------------------------------- src/util/util.c | 67 +++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 + 4 files changed, 93 insertions(+), 60 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4e61e55..06fa319 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -672,6 +672,7 @@ virParseMacAddr; virFileDeletePid; virFindFileInPath; virFileExists; +virFileIsOnNetworkShare; virFileHasSuffix; virFileLinkPointsTo; virFileMakePath; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9140b50..6bbc94b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -46,13 +46,6 @@ #include <sys/ioctl.h> #include <sys/un.h> -#ifdef __linux__ -# include <sys/vfs.h> -# ifndef NFS_SUPER_MAGIC -# define NFS_SUPER_MAGIC 0x6969 -# endif /* NFS_SUPER_MAGIC */ -#endif /* __linux__ */ - #include "virterror_internal.h" #include "logging.h" #include "datatypes.h" @@ -5069,62 +5062,32 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, goto endjob; } -#ifdef __linux__ /* On Linux we can also verify the FS-type of the directory. */ - char *dirpath, *p; - struct statfs st; - int statfs_ret; - - if ((dirpath = strdup(path)) == NULL) { - virReportOOMError(); - goto endjob; - } - - do { - // Try less and less of the path until we get to a - // directory we can stat. Even if we don't have 'x' - // permission on any directory in the path on the NFS - // server (assuming it's NFS), we will be able to stat the - // mount point, and that will properly tell us if the - // fstype is NFS. - - if ((p = strrchr(dirpath, '/')) == NULL) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("Invalid relative path '%s' for domain save file"), - path); - VIR_FREE(dirpath); - goto endjob; - } - - if (p == dirpath) - *(p+1) = '\0'; - else - *p = '\0'; - - statfs_ret = statfs(dirpath, &st); - - } while ((statfs_ret == -1) && (p != dirpath)); - - if (statfs_ret == -1) { - virReportSystemError(errno, - _("Failed to create domain save file " - "'%s': statfs of all elements of path " - "failed"), - path); - VIR_FREE(dirpath); - goto endjob; - } + switch (virFileIsOnNetworkShare(path)) { + case 1: + /* it was on a network share, so we'll continue + * as outlined above + */ + break; + + case -1: + virReportSystemError(errno, + _("Failed to create domain save file " + "'%s': couldn't determine fs type"), + path); + goto endjob; + break; + + case 0: + default: + /* local file - log the error returned by virFileOperation */ + virReportSystemError(rc, + _("Failed to create domain save file '%s'"), + path); + goto endjob; + break; - if (st.f_type != NFS_SUPER_MAGIC) { - virReportSystemError(rc, - _("Failed to create domain save file '%s'" - " (fstype of '%s' is 0x%X"), - path, dirpath, (unsigned int) st.f_type); - VIR_FREE(dirpath); - goto endjob; } - VIR_FREE(dirpath); -#endif /* Retry creating the file as driver->user */ diff --git a/src/util/util.c b/src/util/util.c index 445fd4e..bf6ea30 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -48,6 +48,13 @@ #endif #include "c-ctype.h" +#ifdef __linux__ +# include <sys/vfs.h> +# ifndef NFS_SUPER_MAGIC +# define NFS_SUPER_MAGIC 0x6969 +# endif /* NFS_SUPER_MAGIC */ +#endif /* __linux__ */ + #ifdef HAVE_PATHS_H # include <paths.h> #endif @@ -1258,6 +1265,66 @@ int virFileExists(const char *path) return(0); } +int virFileIsOnNetworkShare(const char *path) +{ +#ifdef __linux__ + char *dirpath, *p; + struct statfs st; + int ret = -1, statfs_ret; + + if ((dirpath = strdup(path)) == NULL) { + virReportOOMError(); + goto error; + } + + do { + /* Try less and less of the path until we get to a + * directory we can stat. Even if we don't have 'x' + * permission on any directory in the path on the NFS + * server (assuming it's NFS), we will be able to stat the + * mount point, and that will properly tell us if the + * fstype is NFS. + */ + + if ((p = strrchr(dirpath, '/')) == NULL) { + virUtilError(VIR_ERR_INVALID_ARG, + _("Invalid relative path '%s'"), path); + goto error; + } + + if (p == dirpath) + *(p+1) = '\0'; + else + *p = '\0'; + + statfs_ret = statfs(dirpath, &st); + + } while ((statfs_ret == -1) && (p != dirpath)); + + if (statfs_ret == -1) { + virReportSystemError(errno, + _("statfs of all elements of path %s failed"), + path); + goto error; + } + + if (st.f_type == NFS_SUPER_MAGIC) { + ret = 1; + } else { + VIR_WARN("fstype of '%s' unrecognized (0x%X)", + dirpath, (unsigned int) st.f_type); + ret = 0; + } + +error: + VIR_FREE(dirpath); + return ret; + +#else + return 1; +#endif +} + # ifndef WIN32 static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, diff --git a/src/util/util.h b/src/util/util.h index 476eac4..494c88a 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -118,6 +118,8 @@ char *virFindFileInPath(const char *file); int virFileExists(const char *path); +int virFileIsOnNetworkShare(const char *path); + char *virFileSanitizePath(const char *path); enum { -- 1.7.1
On Fri, Jun 25, 2010 at 07:42:14AM -0400, Laine Stump wrote:
This code was previously used inline during qemudDomainSaveFlag. This patch moves it into a utility function in preparation for it to be used elsewhere.
How about just switching to use virStorageFileIsSharedFS() ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Jun 25, 2010 at 07:42:14AM -0400, Laine Stump wrote:
This code was previously used inline during qemudDomainSaveFlag. This patch moves it into a utility function in preparation for it to be used elsewhere.
How about just switching to use virStorageFileIsSharedFS() ?
Ah! I either hadn't noticed that function go in, or had forgotten about it. One problem I see is that virStorageFileIsSharedFS doesn't traverse back up the path elements looking for a partial path it can stat. This means it will fail if the file in question (or any of its ancestor directories) isn't stat'able by root. Any objections to me changing virStorageFileIsSharedFS to behave like that?
On Fri, Jun 25, 2010 at 09:09:42AM -0400, Laine Stump wrote:
On Fri, Jun 25, 2010 at 07:42:14AM -0400, Laine Stump wrote:
This code was previously used inline during qemudDomainSaveFlag. This patch moves it into a utility function in preparation for it to be used elsewhere.
How about just switching to use virStorageFileIsSharedFS() ?
Ah! I either hadn't noticed that function go in, or had forgotten about it.
One problem I see is that virStorageFileIsSharedFS doesn't traverse back up the path elements looking for a partial path it can stat. This means it will fail if the file in question (or any of its ancestor directories) isn't stat'able by root.
Any objections to me changing virStorageFileIsSharedFS to behave like that?
Sounds reasonable to me Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
When the saved domain image is on an NFS share, at least some part of domainSetSecurityAllLabel will fail (for example, selinux labels can't be modified). To allow domain restore to still work in this case, just ignore the errors. --- src/qemu/qemu_driver.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6bbc94b..6dbcf6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3368,8 +3368,10 @@ static int qemudStartVMDaemon(virConnectPtr conn, DEBUG0("Generating setting domain security labels (if required)"); if (driver->securityDriver && driver->securityDriver->domainSetSecurityAllLabel && - driver->securityDriver->domainSetSecurityAllLabel(vm, stdin_path) < 0) - goto cleanup; + driver->securityDriver->domainSetSecurityAllLabel(vm, stdin_path) < 0) { + if (virFileIsOnNetworkShare(stdin_path) != 1) + goto cleanup; + } /* Ensure no historical cgroup for this VM is lying around bogus * settings */ -- 1.7.1
participants (2)
-
Daniel P. Berrange -
Laine Stump