[libvirt] [PATCHv4 0/2] qemu: eliminate "Ignoring open failure" (virFileOpenAs)
I believe this version addresses all the issues Eric raised in his review of V3, *except* those that we agreed should be left for a later patch. Patch 2/2 is unchanged all the way since v1. Patch 1/2 has the following changes from V3: * The "last ditch attempt to open the file in the parent process after failing with fork+setuid is done only if VIR_FILE_OPEN_NOFORK is false (meaning that it wasn't already tried). fchmod/fchown is then performed if necessary. * fchmod is only attempted after checking the file's current permissions and seeing that they don't match what is desired. * fchown and fchmod stuff is moved into a helper function that's called from 3 places. * fixed typos, added command about uid & gid no longer being "-1" Items *NOT* addressed: * eliminate log messages in child process * figure out how to have qemuOpenFile call virFileOpenAs just once * switch from use of waitpid to virPidWait() (and use of virPidAbort() to cause the child to terminate when an error is encountered while waiting for an fd from the child. * figure out when it is appropriate to unlink the file on failure. * move virFileOpenAs (and several other functions) to virfile.c
virFileOpenAs previously would only try opening a file as the current user, or as a different user, but wouldn't try both methods in a single call. This made it cumbersome to use as a replacement for open(2). Additionally, it had a lot of historical baggage that led to it being difficult to understand. This patch refactors virFileOpenAs in the following ways: * reorganize the code so that everything dealing with both the parent and child sides of the "fork+setuid+setgid+open" method are in a separate function. This makes the public function easier to understand. * Allow a single call to virFileOpenAs() to first attempt the open as the current user, and if that fails to automatically re-try after doing fork+setuid (if deemed appropriate, i.e. errno indicates it would now be successful, and the file is on a networkFS). This makes it possible (in many, but possibly not all, cases) to drop-in virFileOpenAs() as a replacement for open(2). (NB: currently qemuOpenFile() calls virFileOpenAs() twice, once without forking, then again with forking. That unfortunately can't be changed without at least some discussion of the ramifications, because the requested file permissions are different in each case, which is something that a single call to virFileOpenAs() can't deal with.) * Add a flag so that any fchown() of the file to a different uid:gid is explicitly requested when the function is called, rather than it being implied by the presence of the O_CREAT flag. This just makes for less subtle surprises to consumers. (Commit b1643dc15c5de886fefe56ad18608d65f1325a2c added the check for O_CREAT before forcing ownership. This patch just makes that restriction more explicit.) * If either the uid or gid is specified as "-1", virFileOpenAs will interpret this to mean "the current [gu]id". All current consumers of virFileOpenAs should retain their present behavior (after a few minor changes to their setup code and arguments). --- src/libxl/libxl_driver.c | 4 +- src/qemu/qemu_driver.c | 8 +- src/storage/storage_backend.c | 12 +- src/util/util.c | 352 +++++++++++++++++++++++++++-------------- src/util/util.h | 6 +- 5 files changed, 246 insertions(+), 136 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 41366e4..6cfc5eb 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -216,7 +216,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from, libxlSavefileHeader hdr; char *xml = NULL; - if ((fd = virFileOpenAs(from, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) { + if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) { libxlError(VIR_ERR_OPERATION_FAILED, "%s", _("cannot read domain image")); goto error; @@ -1836,7 +1836,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, } if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR, - getuid(), getgid(), 0)) < 0) { + -1, -1, 0)) < 0) { virReportSystemError(-fd, _("Failed to create domain save file '%s'"), to); goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 79ad06f..bf66687 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2375,6 +2375,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags, bool is_reg = true; bool need_unlink = false; bool bypass_security = false; + unsigned int vfoflags = 0; int fd = -1; uid_t uid = getuid(); gid_t gid = getgid(); @@ -2384,6 +2385,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags, * in the failure case */ if (oflags & O_CREAT) { need_unlink = true; + vfoflags |= VIR_FILE_OPEN_FORCE_OWNER; if (stat(path, &sb) == 0) { is_reg = !!S_ISREG(sb.st_mode); /* If the path is regular file which exists @@ -2404,8 +2406,8 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags, goto cleanup; } } else { - if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, - uid, gid, 0)) < 0) { + if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid, + vfoflags | VIR_FILE_OPEN_NOFORK)) < 0) { /* If we failed as root, and the error was permission-denied (EACCES or EPERM), assume it's on a network-connected share where root access is restricted (eg, root-squashed NFS). If the @@ -2449,7 +2451,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags, if ((fd = virFileOpenAs(path, oflags, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, driver->user, driver->group, - VIR_FILE_OPEN_AS_UID)) < 0) { + vfoflags | VIR_FILE_OPEN_FORK)) < 0) { virReportSystemError(-fd, _("Error from child process creating '%s'"), path); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 306e487..1c22112 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -380,8 +380,6 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, { int ret = -1; int fd = -1; - uid_t uid; - gid_t gid; int operation_flags; virCheckFlags(0, -1); @@ -393,15 +391,15 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } - uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid; - gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid; - operation_flags = VIR_FILE_OPEN_FORCE_PERMS; + operation_flags = VIR_FILE_OPEN_FORCE_MODE | VIR_FILE_OPEN_FORCE_OWNER; if (pool->def->type == VIR_STORAGE_POOL_NETFS) - operation_flags |= VIR_FILE_OPEN_AS_UID; + operation_flags |= VIR_FILE_OPEN_FORK; if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode, uid, gid, + vol->target.perms.mode, + vol->target.perms.uid, + vol->target.perms.gid, operation_flags)) < 0) { virReportSystemError(-fd, _("cannot create path '%s'"), diff --git a/src/util/util.c b/src/util/util.c index 09062f6..7467923 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -70,6 +70,7 @@ #include "logging.h" #include "buf.h" #include "util.h" +#include "storage_file.h" #include "memory.h" #include "threads.h" #include "verify.h" @@ -753,115 +754,59 @@ childerror: _exit(ret); } -/* return -errno on failure, or 0 on success */ +/* virFileOpenForceOwnerMode() - an internal utility function called + * only by virFileOpenAs(). Sets the owner and mode of the file + * opened as "fd" if it's not correct AND the flags say it should be + * forced. */ static int -virFileOpenAsNoFork(const char *path, int openflags, mode_t mode, - uid_t uid, gid_t gid, unsigned int flags) +virFileOpenForceOwnerMode(const char *path, int fd, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { - int fd = -1; - int ret = 0; - - if ((fd = open(path, openflags, mode)) < 0) { - ret = -errno; - virReportSystemError(errno, _("failed to create file '%s'"), - path); - goto error; - } - - /* VIR_FILE_OPEN_AS_UID in flags means we are running in a child process - * owned by uid and gid */ - if (!(flags & VIR_FILE_OPEN_AS_UID)) { - struct stat st; - - if (fstat(fd, &st) == -1) { - ret = -errno; - virReportSystemError(errno, _("stat of '%s' failed"), path); - goto error; - } - if (((st.st_uid != uid) || (st.st_gid != gid)) - && (openflags & O_CREAT) - && (fchown(fd, uid, gid) < 0)) { - ret = -errno; - virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), - path, (unsigned int) uid, (unsigned int) gid); - goto error; - } - } - - if ((flags & VIR_FILE_OPEN_FORCE_PERMS) - && (fchmod(fd, mode) < 0)) { - ret = -errno; - virReportSystemError(errno, - _("cannot set mode of '%s' to %04o"), - path, mode); - goto error; - } - return fd; - -error: - VIR_FORCE_CLOSE(fd); - return ret; -} - -/* return -errno on failure, or 0 on success */ -static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid, - unsigned int flags) { int ret = 0; struct stat st; - if ((mkdir(path, mode) < 0) - && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { - ret = -errno; - virReportSystemError(errno, _("failed to create directory '%s'"), - path); - goto error; - } + if (!(flags & (VIR_FILE_OPEN_FORCE_OWNER | VIR_FILE_OPEN_FORCE_MODE))) + return 0; - if (stat(path, &st) == -1) { + if (fstat(fd, &st) == -1) { ret = -errno; virReportSystemError(errno, _("stat of '%s' failed"), path); - goto error; + return ret; } - if (((st.st_uid != uid) || (st.st_gid != gid)) - && (chown(path, uid, gid) < 0)) { + /* NB: uid:gid are never "-1" (default) at this point - the caller + * has always changed -1 to the value of get[gu]id(). + */ + if ((flags & VIR_FILE_OPEN_FORCE_OWNER) && + ((st.st_uid != uid) || (st.st_gid != gid)) && + (fchown(fd, uid, gid) < 0)) { ret = -errno; - virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), - path, (unsigned int) uid, (unsigned int) gid); - goto error; + virReportSystemError(errno, + _("cannot chown '%s' to (%u, %u)"), + path, (unsigned int) uid, + (unsigned int) gid); + return ret; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) - && (chmod(path, mode) < 0)) { + if ((flags & VIR_FILE_OPEN_FORCE_MODE) && + ((mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != + (st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO))) && + (fchmod(fd, mode) < 0)) { ret = -errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), path, mode); - goto error; + return ret; } -error: return ret; } -/** - * virFileOpenAs: - * @path: file to open or create - * @openflags: flags to pass to open - * @mode: mode to use on creation or when forcing permissions - * @uid: uid that should own file on creation - * @gid: gid that should own file - * @flags: bit-wise or of VIR_FILE_OPEN_* flags - * - * Open @path, and execute an optional callback @hook on the open file - * description. @hook must return 0 on success, or -errno on failure. - * If @flags includes VIR_FILE_OPEN_AS_UID, then open the file while the - * effective user id is @uid (by using a child process); this allows - * one to bypass root-squashing NFS issues. If @flags includes - * VIR_FILE_OPEN_FORCE_PERMS, then ensure that @path has those - * permissions before the callback, even if the file already existed - * with different permissions. The return value (if non-negative) - * is the file descriptor, left open. Return -errno on failure. */ -int -virFileOpenAs(const char *path, int openflags, mode_t mode, - uid_t uid, gid_t gid, unsigned int flags) +/* virFileOpenForked() - an internal utility function called only by + * virFileOpenAs(). It forks, then the child does setuid+setgid to + * given uid:gid and attempts to open the file, while the parent just + * calls recvfd to get the open fd back from the child. returns the + * fd, or -errno if there is an error. */ +static int +virFileOpenForked(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { pid_t pid; int waitret, status, ret = 0; @@ -869,15 +814,8 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, int pair[2] = { -1, -1 }; int forkRet; - if ((!(flags & VIR_FILE_OPEN_AS_UID)) - || (getuid() != 0) - || ((uid == 0) && (gid == 0))) { - flags &= ~VIR_FILE_OPEN_AS_UID; - return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); - } - /* parent is running as root, but caller requested that the - * file be created as some other user and/or group). The + * file be opened as some other user and/or group). The * following dance avoids problems caused by root-squashing * NFS servers. */ @@ -890,28 +828,22 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, } forkRet = virFork(&pid); - - if (pid < 0) { - ret = -errno; - return ret; - } + if (pid < 0) + return -errno; if (pid) { /* parent */ VIR_FORCE_CLOSE(pair[1]); do { - ret = recvfd(pair[0], 0); - } while (ret < 0 && errno == EINTR); + fd = recvfd(pair[0], 0); + } while (fd < 0 && errno == EINTR); + VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */ - if (ret < 0 && errno != EACCES) { + if (fd < 0 && errno != EACCES) { ret = -errno; - VIR_FORCE_CLOSE(pair[0]); while (waitpid(pid, NULL, 0) == -1 && errno == EINTR); - goto parenterror; - } else { - fd = ret; + return ret; } - VIR_FORCE_CLOSE(pair[0]); /* wait for child to complete, and retrieve its exit code */ while ((waitret = waitpid(pid, &status, 0) == -1) @@ -922,31 +854,41 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, _("failed to wait for child creating '%s'"), path); VIR_FORCE_CLOSE(fd); - goto parenterror; + return ret; } if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ VIR_FORCE_CLOSE(fd); - flags &= ~VIR_FILE_OPEN_AS_UID; - return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); + if (flags & VIR_FILE_OPEN_NOFORK) { + /* If we had already tried opening w/o fork+setuid and + * failed, no sense trying again. Just set return the + * original errno that we got at that time (by + * definition, always either EACCES or EPERM - EACCES + * is close enough). + */ + return -EACCES; + } + if ((fd = open(path, openflags, mode)) < 0) + return -errno; + ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); + if (ret < 0) { + VIR_FORCE_CLOSE(fd); + return ret; + } } - if (!ret) - ret = fd; -parenterror: - return ret; + return fd; } - /* child */ + VIR_FORCE_CLOSE(pair[0]); /* preserves errno */ if (forkRet < 0) { /* error encountered and logged in virFork() after the fork. */ ret = -errno; goto childerror; } - VIR_FORCE_CLOSE(pair[0]); /* set desired uid/gid, then attempt to create the file */ @@ -955,10 +897,18 @@ parenterror: goto childerror; } - ret = virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); + if ((fd = open(path, openflags, mode)) < 0) { + ret = -errno; + virReportSystemError(errno, + _("child process failed to create file '%s'"), + path); + goto childerror; + } + + /* File is successfully open. Set permissions if requested. */ + ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); if (ret < 0) goto childerror; - fd = ret; do { ret = sendfd(pair[1], fd); @@ -966,6 +916,8 @@ parenterror: if (ret < 0) { ret = -errno; + virReportSystemError(errno, "%s", + _("child process failed to send fd to parent")); goto childerror; } @@ -975,13 +927,169 @@ childerror: /* XXX This makes assumptions about errno being < 255, which is * not true on Hurd. */ VIR_FORCE_CLOSE(pair[1]); + if (ret < 0) { + VIR_FORCE_CLOSE(fd); + } ret = -ret; if ((ret & 0xff) != ret) { VIR_WARN("unable to pass desired return value %d", ret); ret = 0xff; } _exit(ret); +} + +/** + * virFileOpenAs: + * @path: file to open or create + * @openflags: flags to pass to open + * @mode: mode to use on creation or when forcing permissions + * @uid: uid that should own file on creation + * @gid: gid that should own file + * @flags: bit-wise or of VIR_FILE_OPEN_* flags + * + * Open @path, and return an fd to the open file. @openflags contains + * the flags normally passed to open(2), while those in @flags are + * used internally. If @flags includes VIR_FILE_OPEN_NOFORK, then try + * opening the file while executing with the current uid:gid + * (i.e. don't fork+setuid+setgid before the call to open()). If + * @flags includes VIR_FILE_OPEN_FORK, then try opening the file while + * the effective user id is @uid (by forking a child process); this + * allows one to bypass root-squashing NFS issues; NOFORK is always + * tried before FORK (the absence of both flags is treated identically + * to (VIR_FILE_OPEN_NOFORK | VIR_FILE_OPEN_FORK)). If @flags includes + * VIR_FILE_OPEN_FORCE_OWNER, then ensure that @path is owned by + * uid:gid before returning (even if it already existed with a + * different owner). If @flags includes VIR_FILE_OPEN_FORCE_MODE, + * ensure it has those permissions before returning (again, even if + * the file already existed with different permissions). The return + * value (if non-negative) is the file descriptor, left open. Returns + * -errno on failure. */ +int +virFileOpenAs(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) +{ + int ret = 0, fd = -1; + + /* allow using -1 to mean "current value" */ + if (uid == (uid_t) -1) + uid = getuid(); + if (gid == (gid_t) -1) + gid = getgid(); + + /* treat absence of both flags as presence of both for simpler + * calling. */ + if (!(flags & (VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK))) + flags |= VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK; + + if ((flags & VIR_FILE_OPEN_NOFORK) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + + if ((fd = open(path, openflags, mode)) < 0) { + ret = -errno; + } else { + ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); + if (ret < 0) + goto error; + } + } + + /* If we either 1) didn't try opening as current user at all, or + * 2) failed, and errno/virStorageFileIsSharedFS indicate we might + * be successful if we try as a different uid, then try doing + * fork+setuid+setgid before opening. + */ + if ((fd < 0) && (flags & VIR_FILE_OPEN_FORK)) { + + if (ret < 0) { + /* An open(2) that failed due to insufficient permissions + * could return one or the other of these depending on OS + * version and circumstances. Any other errno indicates a + * problem that couldn't be remedied by fork+setuid + * anyway. */ + if (ret != -EACCES && ret != -EPERM) + goto error; + + /* On Linux we can also verify the FS-type of the + * directory. (this is a NOP on other platforms). */ + switch (virStorageFileIsSharedFS(path)) { + case 1: + /* it was on a network share, so we'll re-try */ + break; + case -1: + /* failure detecting fstype */ + virReportSystemError(errno, _("couldn't determine fs type of" + "of mount containing '%s'"), path); + goto error; + case 0: + default: + /* file isn't on a recognized network FS */ + goto error; + } + } + + /* passed all prerequisites - retry the open w/fork+setuid */ + if ((fd = virFileOpenForked(path, openflags, mode, uid, gid, flags)) < 0) { + ret = fd; + fd = -1; + goto error; + } + } + + /* File is successfully opened */ + return fd; + +error: + if (fd < 0) { + /* whoever failed the open last has already set ret = -errno */ + virReportSystemError(-ret, openflags & O_CREAT + ? _("failed to create file '%s'") + : _("failed to open file '%s'"), + path); + } else { + /* some other failure after the open succeeded */ + VIR_FORCE_CLOSE(fd); + } + return ret; +} + +/* return -errno on failure, or 0 on success */ +static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) { + int ret = 0; + struct stat st; + + if ((mkdir(path, mode) < 0) + && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { + ret = -errno; + virReportSystemError(errno, _("failed to create directory '%s'"), + path); + goto error; + } + + if (stat(path, &st) == -1) { + ret = -errno; + virReportSystemError(errno, _("stat of '%s' failed"), path); + goto error; + } + if (((st.st_uid != uid) || (st.st_gid != gid)) + && (chown(path, uid, gid) < 0)) { + ret = -errno; + virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), + path, (unsigned int) uid, (unsigned int) gid); + goto error; + } + if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + && (chmod(path, mode) < 0)) { + ret = -errno; + virReportSystemError(errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto error; + } +error: + return ret; } /* return -errno on failure, or 0 on success */ diff --git a/src/util/util.h b/src/util/util.h index f62cb42..5c945cc 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -92,8 +92,10 @@ char *virFileSanitizePath(const char *path); enum { VIR_FILE_OPEN_NONE = 0, - VIR_FILE_OPEN_AS_UID = (1 << 0), - VIR_FILE_OPEN_FORCE_PERMS = (1 << 1), + VIR_FILE_OPEN_NOFORK = (1 << 0), + VIR_FILE_OPEN_FORK = (1 << 1), + VIR_FILE_OPEN_FORCE_MODE = (1 << 2), + VIR_FILE_OPEN_FORCE_OWNER = (1 << 3), }; int virFileAccessibleAs(const char *path, int mode, uid_t uid, gid_t gid) -- 1.7.7.5
On 02/01/2012 11:36 PM, Laine Stump wrote:
virFileOpenAs previously would only try opening a file as the current user, or as a different user, but wouldn't try both methods in a single call. This made it cumbersome to use as a replacement for open(2). Additionally, it had a lot of historical baggage that led to it being difficult to understand.
This patch refactors virFileOpenAs in the following ways:
All current consumers of virFileOpenAs should retain their present behavior (after a few minor changes to their setup code and arguments).
Yep, sure looks like it.
--- src/libxl/libxl_driver.c | 4 +- src/qemu/qemu_driver.c | 8 +- src/storage/storage_backend.c | 12 +- src/util/util.c | 352 +++++++++++++++++++++++++++-------------- src/util/util.h | 6 +- 5 files changed, 246 insertions(+), 136 deletions(-)
I think we've hit a winner for refactoring it into something that can be further modified while investigating those modifications, making it more powerful for new uses, and without breaking behavior in the refactor.
+/* virFileOpenForceOwnerMode() - an internal utility function called + * only by virFileOpenAs(). Sets the owner and mode of the file + * opened as "fd" if it's not correct AND the flags say it should be + * forced. */ static int -virFileOpenAsNoFork(const char *path, int openflags, mode_t mode, - uid_t uid, gid_t gid, unsigned int flags) +virFileOpenForceOwnerMode(const char *path, int fd, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) {
- if ((flags & VIR_DIR_CREATE_FORCE_PERMS) - && (chmod(path, mode) < 0)) { + if ((flags & VIR_FILE_OPEN_FORCE_MODE) && + ((mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != + (st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO))) &&
Technically, this limits us to a mask of 00777,
+ (fchmod(fd, mode) < 0)) { ret = -errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"),
while this error message implied that we could request a mode with mask 07777. But since none of the callers are passing sticky bits, I think we can adjust that later if we ever find a reason that we need bits from 07000 modified, and leave well enough alone in this patch.
+/* virFileOpenForked() - an internal utility function called only by + * virFileOpenAs(). It forks, then the child does setuid+setgid to + * given uid:gid and attempts to open the file, while the parent just + * calls recvfd to get the open fd back from the child. returns the + * fd, or -errno if there is an error. */ +static int +virFileOpenForked(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { pid_t pid;
forkRet = virFork(&pid); - - if (pid < 0) { - ret = -errno; - return ret; - } + if (pid < 0) + return -errno;
if (pid) { /* parent */
You know, the diff for this patch is so crazy already that we might as well make maintenance slightly easier. That is, right now, we have: fork if fail return error if parent { wait for child do stuff, which has several return calls return } child do stuff, which might go to childerror childerr: _exit But sequentially, since the parent is waiting on the child, it might be easier to read as: fork if child { do stuff, which might go to childerror childerror: _exit } parent if fork failed, go to cleanup wait for child do more stuff, which might go to cleanup cleanup: return In other words, if you want to rearrange this section of code, now might be a good time to do it, and I wouldn't even mind if you squashed it in to this one rather than using a separate patch. But that's all cosmetic, it wouldn't change the code you actually have.
if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ VIR_FORCE_CLOSE(fd); - flags &= ~VIR_FILE_OPEN_AS_UID; - return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); + if (flags & VIR_FILE_OPEN_NOFORK) { + /* If we had already tried opening w/o fork+setuid and + * failed, no sense trying again. Just set return the + * original errno that we got at that time (by + * definition, always either EACCES or EPERM - EACCES + * is close enough). + */ + return -EACCES;
Fair enough. I don't think the slight loss in information will hurt; the end result is still a reasonable failure message. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/03/2012 03:41 PM, Eric Blake wrote:
> On 02/01/2012 11:36 PM, Laine Stump wrote:
>> virFileOpenAs previously would only try opening a file as the current
>> user, or as a different user, but wouldn't try both methods in a
>> single call. This made it cumbersome to use as a replacement for
>> open(2). Additionally, it had a lot of historical baggage that led to
>> it being difficult to understand.
>>
>> This patch refactors virFileOpenAs in the following ways:
>> All current consumers of virFileOpenAs should retain their present
>> behavior (after a few minor changes to their setup code and
>> arguments).
> Yep, sure looks like it.
>
>> ---
>> src/libxl/libxl_driver.c | 4 +-
>> src/qemu/qemu_driver.c | 8 +-
>> src/storage/storage_backend.c | 12 +-
>> src/util/util.c | 352 +++++++++++++++++++++++++++--------------
>> src/util/util.h | 6 +-
>> 5 files changed, 246 insertions(+), 136 deletions(-)
> I think we've hit a winner for refactoring it into something that can be
> further modified while investigating those modifications, making it more
> powerful for new uses, and without breaking behavior in the refactor.
>
>> +/* virFileOpenForceOwnerMode() - an internal utility function called
>> + * only by virFileOpenAs(). Sets the owner and mode of the file
>> + * opened as "fd" if it's not correct AND the flags say it should be
>> + * forced. */
>> static int
>> -virFileOpenAsNoFork(const char *path, int openflags, mode_t mode,
>> - uid_t uid, gid_t gid, unsigned int flags)
>> +virFileOpenForceOwnerMode(const char *path, int fd, mode_t mode,
>> + uid_t uid, gid_t gid, unsigned int flags)
>> {
>> - if ((flags& VIR_DIR_CREATE_FORCE_PERMS)
>> -&& (chmod(path, mode)< 0)) {
>> + if ((flags& VIR_FILE_OPEN_FORCE_MODE)&&
>> + ((mode& (S_IRWXU|S_IRWXG|S_IRWXO)) !=
>> + (st.st_mode& (S_IRWXU|S_IRWXG|S_IRWXO)))&&
> Technically, this limits us to a mask of 00777,
>
>> + (fchmod(fd, mode)< 0)) {
>> ret = -errno;
>> virReportSystemError(errno,
>> _("cannot set mode of '%s' to %04o"),
> while this error message implied that we could request a mode with mask
> 07777. But since none of the callers are passing sticky bits, I think
> we can adjust that later if we ever find a reason that we need bits from
> 07000 modified, and leave well enough alone in this patch.
>
>> +/* virFileOpenForked() - an internal utility function called only by
>> + * virFileOpenAs(). It forks, then the child does setuid+setgid to
>> + * given uid:gid and attempts to open the file, while the parent just
>> + * calls recvfd to get the open fd back from the child. returns the
>> + * fd, or -errno if there is an error. */
>> +static int
>> +virFileOpenForked(const char *path, int openflags, mode_t mode,
>> + uid_t uid, gid_t gid, unsigned int flags)
>> {
>> pid_t pid;
>> forkRet = virFork(&pid);
>> -
>> - if (pid< 0) {
>> - ret = -errno;
>> - return ret;
>> - }
>> + if (pid< 0)
>> + return -errno;
>>
>> if (pid) { /* parent */
> You know, the diff for this patch is so crazy already that we might as
> well make maintenance slightly easier. That is, right now, we have:
>
> fork
> if fail
> return error
> if parent {
> wait for child
> do stuff, which has several return calls
> return
> }
> child
> do stuff, which might go to childerror
> childerr:
> _exit
>
> But sequentially, since the parent is waiting on the child, it might be
> easier to read as:
>
> fork
> if child {
> do stuff, which might go to childerror
> childerror:
> _exit
> }
> parent
> if fork failed, go to cleanup
> wait for child
> do more stuff, which might go to cleanup
> cleanup:
> return
>
> In other words, if you want to rearrange this section of code, now might
> be a good time to do it, and I wouldn't even mind if you squashed it in
> to this one rather than using a separate patch. But that's all
> cosmetic, it wouldn't change the code you actually have.
>
>
>> if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES ||
>> fd == -1) {
>> /* fall back to the simpler method, which works better in
>> * some cases */
>> VIR_FORCE_CLOSE(fd);
>> - flags&= ~VIR_FILE_OPEN_AS_UID;
>> - return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
>> + if (flags& VIR_FILE_OPEN_NOFORK) {
>> + /* If we had already tried opening w/o fork+setuid and
>> + * failed, no sense trying again. Just set return the
>> + * original errno that we got at that time (by
>> + * definition, always either EACCES or EPERM - EACCES
>> + * is close enough).
>> + */
>> + return -EACCES;
> Fair enough. I don't think the slight loss in information will hurt;
> the end result is still a reasonable failure message.
>
> ACK.
>
reordered and pushed. Thanks!
This eliminates the warning message reported in: https://bugzilla.redhat.com/show_bug.cgi?id=624447 It was caused by a failure to open an image file that is not accessible by root (the uid libvirtd is running as) because it's on a root-squash NFS share, owned by a different user, with permissions of 660 (or maybe 600). The solution is to use virFileOpenAs() rather than open(). The codepath that generates the error is during qemuSetupDiskCGroup(), but the actual open() is in a lower-level generic function called from many places (virDomainDiskDefForeachPath), so some other pieces of the code were touched just to add dummy (or possibly useful) uid and gid arguments. Eliminating this warning message has the nice side effect that the requested operation may even succeed (which in this case isn't necessary, but shouldn't hurt anything either). --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 1 + src/qemu/qemu_cgroup.c | 2 ++ src/security/security_dac.c | 1 + src/security/security_selinux.c | 7 +++++++ src/security/virt-aa-helper.c | 6 +++++- 6 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35cb7a4..83971dc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13491,6 +13491,7 @@ done: int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, bool allowProbing, bool ignoreOpenFailure, + uid_t uid, gid_t gid, virDomainDiskDefPathIterator iter, void *opaque) { @@ -13547,15 +13548,14 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, goto cleanup; } - if ((fd = open(path, O_RDONLY)) < 0) { + if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { if (ignoreOpenFailure) { char ebuf[1024]; VIR_WARN("Ignoring open failure on %s: %s", path, - virStrerror(errno, ebuf, sizeof(ebuf))); + virStrerror(-fd, ebuf, sizeof(ebuf))); break; } else { - virReportSystemError(errno, - _("unable to open disk path %s"), + virReportSystemError(-fd, _("unable to open disk path %s"), path); goto cleanup; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 503684f..eb35afe 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1961,6 +1961,7 @@ typedef int (*virDomainDiskDefPathIterator)(virDomainDiskDefPtr disk, int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, bool allowProbing, bool ignoreOpenFailure, + uid_t uid, gid_t gid, virDomainDiskDefPathIterator iter, void *opaque); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2d970d6..a07b6cd 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -96,6 +96,7 @@ int qemuSetupDiskCgroup(struct qemud_driver *driver, return virDomainDiskDefForeachPath(disk, driver->allowDiskFormatProbing, true, + driver->user, driver->group, qemuSetupDiskPathAllow, &data); } @@ -137,6 +138,7 @@ int qemuTeardownDiskCgroup(struct qemud_driver *driver, return virDomainDiskDefForeachPath(disk, driver->allowDiskFormatProbing, true, + driver->user, driver->group, qemuTeardownDiskPathDeny, &data); } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2fb4a14..3e1a72f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -186,6 +186,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, return virDomainDiskDefForeachPath(disk, virSecurityManagerGetAllowDiskFormatProbing(mgr), false, + priv->user, priv->group, virSecurityDACSetSecurityFileLabel, mgr); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index cb41a17..139aab8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -671,9 +671,16 @@ SELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0; + /* XXX On one hand, it would be nice to have the driver's uid:gid + * here so we could retry opens with it. On the other hand, it + * probably doesn't matter because in practice that's only useful + * for files on root-squashed NFS shares, and NFS doesn't properly + * support selinux anyway. + */ return virDomainDiskDefForeachPath(disk, allowDiskFormatProbing, true, + -1, -1, /* current process uid:gid */ SELinuxSetSecurityFileLabel, secdef); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index b484a20..1971f40 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -910,10 +910,14 @@ get_files(vahControl * ctl) /* XXX passing ignoreOpenFailure = true to get back to the behavior * from before using virDomainDiskDefForeachPath. actually we should * be passing ignoreOpenFailure = false and handle open errors more - * careful than just ignoring them */ + * careful than just ignoring them. + * XXX2 - if we knew the qemu user:group here we could send it in + * so that the open could be re-tried as that user:group. + */ int ret = virDomainDiskDefForeachPath(ctl->def->disks[i], ctl->allowDiskFormatProbing, true, + -1, -1 /* current uid:gid */ add_file_path, &buf); if (ret != 0) -- 1.7.7.5
On 02/01/2012 11:36 PM, Laine Stump wrote:
This eliminates the warning message reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=624447
It was caused by a failure to open an image file that is not accessible by root (the uid libvirtd is running as) because it's on a root-squash NFS share, owned by a different user, with permissions of 660 (or maybe 600).
The solution is to use virFileOpenAs() rather than open(). The codepath that generates the error is during qemuSetupDiskCGroup(), but the actual open() is in a lower-level generic function called from many places (virDomainDiskDefForeachPath), so some other pieces of the code were touched just to add dummy (or possibly useful) uid and gid arguments.
Eliminating this warning message has the nice side effect that the requested operation may even succeed (which in this case isn't necessary, but shouldn't hurt anything either). --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 1 + src/qemu/qemu_cgroup.c | 2 ++ src/security/security_dac.c | 1 + src/security/security_selinux.c | 7 +++++++ src/security/virt-aa-helper.c | 6 +++++- 6 files changed, 20 insertions(+), 5 deletions(-)
After all that churn on 1/2, this one has just been patiently waiting, unchanged. That shows that once we get a good interface, using it is easier. ACK.
+++ b/src/security/security_selinux.c @@ -671,9 +671,16 @@ SELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0;
+ /* XXX On one hand, it would be nice to have the driver's uid:gid + * here so we could retry opens with it. On the other hand, it + * probably doesn't matter because in practice that's only useful + * for files on root-squashed NFS shares, and NFS doesn't properly + * support selinux anyway.
Too true. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/03/2012 03:45 PM, Eric Blake wrote:
On 02/01/2012 11:36 PM, Laine Stump wrote:
This eliminates the warning message reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=624447
It was caused by a failure to open an image file that is not accessible by root (the uid libvirtd is running as) because it's on a root-squash NFS share, owned by a different user, with permissions of 660 (or maybe 600).
The solution is to use virFileOpenAs() rather than open(). The codepath that generates the error is during qemuSetupDiskCGroup(), but the actual open() is in a lower-level generic function called from many places (virDomainDiskDefForeachPath), so some other pieces of the code were touched just to add dummy (or possibly useful) uid and gid arguments.
Eliminating this warning message has the nice side effect that the requested operation may even succeed (which in this case isn't necessary, but shouldn't hurt anything either). --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 1 + src/qemu/qemu_cgroup.c | 2 ++ src/security/security_dac.c | 1 + src/security/security_selinux.c | 7 +++++++ src/security/virt-aa-helper.c | 6 +++++- 6 files changed, 20 insertions(+), 5 deletions(-) After all that churn on 1/2, this one has just been patiently waiting, unchanged. That shows that once we get a good interface, using it is easier.
ACK.
And hopefully we can use it to clear up some other similar problems. Pushed. Thanks!
participants (2)
-
Eric Blake -
Laine Stump