[libvirt] [PATCH 0/4] Resolve a few NFS root-squash related issues
The following series of patches resolve a few issues found within and because of an NFS root-squash environment. Patch 1: No real change - mostly to get data on which path was failing Patch 2: Create a virFileUnlink() API which will essentially follow the code that virFileCreateAs[Forked] and virDirCreate[NoFork] have taken with respect to running the 'unlink' in a fork'd process under the uid/gid used to create the volume. Patch 3: Commit id '7c2d65dde2' used the default mode to create a file when "vol->target.perms->mode" was set rather than checking it against -1 in virStorageBackendCreateRaw when calling virFileOpenAs. The logic was change to follow other changes which check "->mode == -1", then use default, else use "->mode" value. Patch 4: Commit id '155ca616' added a check and call to a storage driver 'refreshVol' API. Unfortunately if it failed, proper cleanup wasn't done leaving 'voldef' set and already in the pool list. The cleanup path would then delete the memory eventually leading to a crash or corruption for an ensuing refresh or list pool. For an NFS root squash environment with incorrect XML to create the file (either wrong uid/gid or mode value), the refreshVol would fail since it tried to run as root. On such a failure, it was possible that the volume remained in the NFS pool when it should have been deleted (unless root could delete the file). The last 3 patches are very much so related and intertwined. Fortunately it's a very limited environment. John Ferlan (4): virfile: Add error for root squash change mode failure virfile: Introduce virFileUnlink storage: Correct the 'mode' check storage: Handle failure from refreshVol src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 8 +-- src/storage/storage_backend_fs.c | 3 +- src/storage/storage_driver.c | 6 ++- src/util/virfile.c | 113 ++++++++++++++++++++++++++++++++++++++- src/util/virfile.h | 1 + 6 files changed, 126 insertions(+), 6 deletions(-) -- 2.1.0
This will only be seen when debugging, but in order to help determine whether a virFileOpenForceOwnerMode failed during an NFS root-squash volume/file creation, add an error message from the child. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 5f64186..3abef05 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2113,8 +2113,13 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, /* File is successfully open. Set permissions if requested. */ ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); - if (ret < 0) + if (ret < 0) { + ret = -errno; + virReportSystemError(errno, + _("child process failed to force owner mode file '%s'"), + path); goto childerror; + } do { ret = sendfd(pair[1], fd); -- 2.1.0
In an NFS root-squashed environment the 'vol-delete' command will fail to 'unlink' the target volume since it was created under a different uid:gid. This code continues the concepts introduced in virFileOpenForked and virDirCreate[NoFork] with respect to running the unlink command under the uid/gid of the child. Unlike the other two, don't retry on EACCES (that's why we're here doing this now). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 3 +- src/util/virfile.c | 106 +++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 1 + 4 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d57bf5b..a96c985 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1462,6 +1462,7 @@ virFileSanitizePath; virFileSkipRoot; virFileStripSuffix; virFileTouch; +virFileUnlink; virFileUnlock; virFileUpdatePerm; virFileWaitForDevices; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c0ea1df..f41a41e 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1203,7 +1203,8 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, switch ((virStorageVolType) vol->type) { case VIR_STORAGE_VOL_FILE: - if (unlink(vol->target.path) < 0) { + if (virFileUnlink(vol->target.path, vol->target.perms->uid, + vol->target.perms->gid) < 0) { /* Silently ignore failures where the vol has already gone away */ if (errno != ENOENT) { virReportSystemError(errno, diff --git a/src/util/virfile.c b/src/util/virfile.c index 3abef05..e3c00ef 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,6 +2311,112 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, return ret; } + +/* virFileUnlink: + * @path: file to unlink + * @uid: uid that was used to create the file (not required) + * @gid: gid that was used to create the file (not required) + * + * If a file/volume was created in an NFS root-squash environment, + * then we must 'unlink' the file in the same environment. Unlike + * the virFileOpenAs[Forked] and virDirCreate[NoFork], this code + * takes no extra flags and does not bother with EACCES failures + * from the child. + */ +int +virFileUnlink(const char *path, + uid_t uid, + gid_t gid) +{ + pid_t pid; + int waitret; + int status, ret = 0; + gid_t *groups; + int ngroups; + + /* If not running as root or if a non explicit uid/gid was being used for + * the file/volume, then use unlink directly + */ + if ((geteuid() != 0) || + ((uid == (uid_t) -1) && (gid == (gid_t) -1))) + return unlink(path); + + /* Otherwise, we have to deal with the NFS root-squash craziness + * to run under the uid/gid that created the volume in order to + * perform the unlink of the volume. + */ + if (uid == (uid_t) -1) + uid = geteuid(); + if (gid == (gid_t) -1) + gid = getegid(); + + ngroups = virGetGroupList(uid, gid, &groups); + if (ngroups < 0) + return -errno; + + pid = virFork(); + + if (pid < 0) { + ret = -errno; + VIR_FREE(groups); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + VIR_FREE(groups); + + while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR); + if (waitret == -1) { + ret = -errno; + virReportSystemError(errno, + _("failed to wait for child unlinking '%s'"), + path); + goto parenterror; + } + + /* + * If waitpid succeeded, but if the child exited abnormally or + * reported non-zero status, report failure + */ + if (!WIFEXITED(status) || (WEXITSTATUS(status)) != 0) { + char *msg = virProcessTranslateStatus(status); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("child failed to unlink '%s': %s"), + path, msg); + VIR_FREE(msg); + if (WIFEXITED(status)) + ret = -WEXITSTATUS(status); + else + ret = -EACCES; + } + + parenterror: + return ret; + } + + /* child */ + + /* set desired uid/gid, then attempt to unlink the file */ + if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { + ret = errno; + goto childerror; + } + + if (unlink(path) < 0) { + ret = errno; + goto childerror; + } + + childerror: + if ((ret & 0xff) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xff; + } + _exit(ret); +} + + /* return -errno on failure, or 0 on success */ static int virDirCreateNoFork(const char *path, diff --git a/src/util/virfile.h b/src/util/virfile.h index 2d27e89..797ca65 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -219,6 +219,7 @@ int virFileOpenAs(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virFileUnlink(const char *path, uid_t uid, gid_t gid); enum { VIR_DIR_CREATE_NONE = 0, -- 2.1.0
Commit id '7c2d65dde2' changed the default value of mode to be -1 if not supplied in the XML, which should cause creation of the volume using the default mode of VIR_STORAGE_DEFAULT_VOL_PERM_MODE; however, the check made was whether mode was '0' or not to use default or provided value. This patch fixes the issue to check if the 'mode' was provided in the XML and use that value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0418473..6a3146c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -486,6 +486,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, int fd = -1; int operation_flags; bool reflink_copy = false; + mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | VIR_STORAGE_VOL_CREATE_REFLINK, @@ -518,11 +519,12 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if (pool->def->type == VIR_STORAGE_POOL_NETFS) operation_flags |= VIR_FILE_OPEN_FORK; + if (vol->target.perms->mode != (mode_t) -1) + open_mode = vol->target.perms->mode; + if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - (vol->target.perms->mode ? - VIR_STORAGE_DEFAULT_VOL_PERM_MODE : - vol->target.perms->mode), + open_mode, vol->target.perms->uid, vol->target.perms->gid, operation_flags)) < 0) { -- 2.1.0
In an NFS root-squash environment it was possible that if the just created volume from XML wasn't properly created with the right uid/gid and/or mode, then the followup refreshVol will fail to open the volume in order to get the allocation/capacity values. This would leave the volume still on the server and cause a libvirtd crash because 'voldef' would be in the pool list, but the cleanup code would free it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ea7e0f3..0494e5d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1867,8 +1867,12 @@ storageVolCreateXML(virStoragePoolPtr obj, } if (backend->refreshVol && - backend->refreshVol(obj->conn, pool, voldef) < 0) + backend->refreshVol(obj->conn, pool, voldef) < 0) { + storageVolDeleteInternal(volobj, backend, pool, voldef, + 0, false); + voldef = NULL; goto cleanup; + } /* Update pool metadata ignoring the disk backend since * it updates the pool values. -- 2.1.0
On Tue, Sep 01, 2015 at 08:55:58PM -0400, John Ferlan wrote:
In an NFS root-squash environment it was possible that if the just created volume from XML wasn't properly created with the right uid/gid and/or mode, then the followup refreshVol will fail to open the volume in order to get the allocation/capacity values. This would leave the volume still on the server and cause a libvirtd crash because 'voldef' would be in the pool list, but the cleanup code would free it.
It would be nice to blame the commit that broke this, released in 1.2.14: commit 155ca616eb231181f6978efc9e3a1eb0eb60af8a Allow creating volumes with a backing store but no capacity (preferably without mentioning the author's name ;) Also, is there a bug that can be made public and linked here? Jan
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ea7e0f3..0494e5d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1867,8 +1867,12 @@ storageVolCreateXML(virStoragePoolPtr obj, }
if (backend->refreshVol && - backend->refreshVol(obj->conn, pool, voldef) < 0) + backend->refreshVol(obj->conn, pool, voldef) < 0) { + storageVolDeleteInternal(volobj, backend, pool, voldef, + 0, false); + voldef = NULL; goto cleanup; + }
/* Update pool metadata ignoring the disk backend since * it updates the pool values. -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 09/02/2015 08:25 AM, Ján Tomko wrote:
On Tue, Sep 01, 2015 at 08:55:58PM -0400, John Ferlan wrote:
In an NFS root-squash environment it was possible that if the just created volume from XML wasn't properly created with the right uid/gid and/or mode, then the followup refreshVol will fail to open the volume in order to get the allocation/capacity values. This would leave the volume still on the server and cause a libvirtd crash because 'voldef' would be in the pool list, but the cleanup code would free it.
It would be nice to blame the commit that broke this, released in 1.2.14: commit 155ca616eb231181f6978efc9e3a1eb0eb60af8a Allow creating volumes with a backing store but no capacity (preferably without mentioning the author's name ;)
Oh right - I did it in my writeup but not the commit message... I'll add before pushing. Although without patch 3, getting a failure from refreshVol was perhaps less likely, but not impossible.
Also, is there a bug that can be made public and linked here?
There is a bz, but it's not public (yet) - the process is ongoing. John
On 09/02/2015 06:57 AM, John Ferlan wrote:
On 09/02/2015 08:25 AM, Ján Tomko wrote:
On Tue, Sep 01, 2015 at 08:55:58PM -0400, John Ferlan wrote:
In an NFS root-squash environment it was possible that if the just created volume from XML wasn't properly created with the right uid/gid and/or mode, then the followup refreshVol will fail to open the volume in order to get the allocation/capacity values. This would leave the volume still on the server and cause a libvirtd crash because 'voldef' would be in the pool list, but the cleanup code would free it.
It would be nice to blame the commit that broke this, released in 1.2.14: commit 155ca616eb231181f6978efc9e3a1eb0eb60af8a Allow creating volumes with a backing store but no capacity (preferably without mentioning the author's name ;)
Oh right - I did it in my writeup but not the commit message... I'll add before pushing. Although without patch 3, getting a failure from refreshVol was perhaps less likely, but not impossible.
Also, is there a bug that can be made public and linked here?
There is a bz, but it's not public (yet) - the process is ongoing.
The issue has been assigned CVE-2015-5247. John took correct steps of first informing the libvirt-security list where we discussed the effect of the bug (the CVE is there mainly if you use fine-grained ACLs); and I am now in the process of writing up a proper Libvirt Security Notice as well as helping backport these patches to affected branches. We'll add signed CVE- tags to libvirt.git before all is said and done. It missed the 1.2.19 release, so these will be the first patches on the new v1.2.19-maint branch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Sep 01, 2015 at 08:55:54PM -0400, John Ferlan wrote:
The following series of patches resolve a few issues found within and because of an NFS root-squash environment.
Patch 1: No real change - mostly to get data on which path was failing
Patch 2: Create a virFileUnlink() API which will essentially follow the code that virFileCreateAs[Forked] and virDirCreate[NoFork] have taken with respect to running the 'unlink' in a fork'd process under the uid/gid used to create the volume.
Patch 3: Commit id '7c2d65dde2' used the default mode to create a file when "vol->target.perms->mode" was set rather than checking it against -1 in virStorageBackendCreateRaw when calling virFileOpenAs. The logic was change to follow other changes which check "->mode == -1", then use default, else use "->mode" value.
Patch 4: Commit id '155ca616' added a check and call to a storage driver 'refreshVol' API. Unfortunately if it failed, proper cleanup wasn't done leaving 'voldef' set and already in the pool list. The cleanup path would then delete the memory eventually leading to a crash or corruption for an ensuing refresh or list pool. For an NFS root squash environment with incorrect XML to create the file (either wrong uid/gid or mode value), the refreshVol would fail since it tried to run as root. On such a failure, it was possible that the volume remained in the NFS pool when it should have been deleted (unless root could delete the file).
The last 3 patches are very much so related and intertwined. Fortunately it's a very limited environment.
John Ferlan (4): virfile: Add error for root squash change mode failure virfile: Introduce virFileUnlink storage: Correct the 'mode' check storage: Handle failure from refreshVol
src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 8 +-- src/storage/storage_backend_fs.c | 3 +- src/storage/storage_driver.c | 6 ++- src/util/virfile.c | 113 ++++++++++++++++++++++++++++++++++++++- src/util/virfile.h | 1 + 6 files changed, 126 insertions(+), 6 deletions(-)
ACK series Jan
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko