Re: [libvirt] [PATCH 2/6] security_dac: Resolve virSecurityDACSetOwnershipInternal const correctness

The code at the very bottom of the DAC secdriver that calls chown() should be fine with read-only data. If something needs to be prepared it should have been done beforehand. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++-------- src/security/security_dac.c | 4 ++-- src/security/security_manager.h | 2 +- src/storage/storage_backend.h | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_driver.c | 6 +++--- src/storage/storage_driver.h | 4 ++-- src/util/virstoragefile.c | 2 +- src/util/virstoragefile.h | 2 +- 10 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a464337e..f68b6ff30 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -329,10 +329,11 @@ qemuAutostartDomains(virQEMUDriverPtr driver) static int -qemuSecurityChownCallback(virStorageSourcePtr src, +qemuSecurityChownCallback(const virStorageSource *storage, uid_t uid, gid_t gid) { + virStorageSourcePtr src = NULL;

struct stat sb; int save_errno = 0; int ret = -1; @@ -340,26 +341,35 @@ qemuSecurityChownCallback(virStorageSourcePtr src, if (!virStorageFileSupportsSecurityDriver(src))

return 0; + if (!(src = virStorageSourceCopy(storage, false))) + goto cleanup; +

if (virStorageSourceIsLocalStorage(src)) { /* use direct chmod for local files so that the file doesn't * need to be initialized */ - if (!src->path) - return 0; + if (!src->path) { + ret = 0; + goto cleanup; + } if (stat(src->path, &sb) >= 0) { if (sb.st_uid == uid && sb.st_gid == gid) { /* It's alright, there's nothing to change anyway. */ - return 0; + ret = 0; + goto cleanup; } } - return chown(src->path, uid, gid); + ret = chown(src->path, uid, gid); + goto cleanup; }

/* storage file init reports errors, return -2 on failure */ - if (virStorageFileInit(src) < 0) - return -2; + if (virStorageFileInit(src) < 0) { + ret = -2; + goto cleanup; + } if (virStorageFileChown(src, uid, gid) < 0) { save_errno = errno; @@ -370,7 +380,9 @@ qemuSecurityChownCallback(virStorageSourcePtr src, cleanup: virStorageFileDeinit(src); - errno = save_errno; + virStorageSourceFree(src); + if (save_errno) + errno = save_errno;

return ret; } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 649219e52..b6f75fe1d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -279,8 +279,8 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) } static int -virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, - virStorageSourcePtr src, +virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, + const virStorageSource *src, const char *path, uid_t uid, gid_t gid) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 0d22cb15f..80fceddc8 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -62,7 +62,7 @@ int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, * @src. The callback shall return 0 on success, -1 on error and errno set (no * libvirt error reported) OR -2 and a libvirt error reported. */ typedef int -(*virSecurityManagerDACChownCallback)(virStorageSourcePtr src, +(*virSecurityManagerDACChownCallback)(const virStorageSource *src, uid_t uid, gid_t gid); diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 28e1a6517..82fbbbf6d 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -285,7 +285,7 @@ typedef int int mode); typedef int -(*virStorageFileBackendChown)(virStorageSourcePtr src, +(*virStorageFileBackendChown)(const virStorageSource *src, uid_t uid, gid_t gid); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de0e8d57d..9de0c17c0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1600,7 +1600,7 @@ virStorageFileBackendFileAccess(virStorageSourcePtr src, static int -virStorageFileBackendFileChown(virStorageSourcePtr src, +virStorageFileBackendFileChown(const virStorageSource *src, uid_t uid, gid_t gid) { diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8e867043e..ae0611543 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -809,7 +809,7 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) static int -virStorageFileBackendGlusterChown(virStorageSourcePtr src, +virStorageFileBackendGlusterChown(const virStorageSource *src, uid_t uid, gid_t gid) { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a79acc6f0..1ac07dbf2 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2832,7 +2832,7 @@ int storageRegister(void) /* ----------- file handlers cooperating with storage driver --------------- */ static bool -virStorageFileIsInitialized(virStorageSourcePtr src) +virStorageFileIsInitialized(const virStorageSource *src) { return src && src->drv; } @@ -2872,7 +2872,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) * driver to perform labelling */ bool -virStorageFileSupportsSecurityDriver(virStorageSourcePtr src) +virStorageFileSupportsSecurityDriver(const virStorageSource *src) { int actualType; virStorageFileBackendPtr backend; @@ -3163,7 +3163,7 @@ virStorageFileAccess(virStorageSourcePtr src, * by libvirt storage backend. */ int -virStorageFileChown(virStorageSourcePtr src, +virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid) { diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 3f2549da5..682c9ff82 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -44,9 +44,9 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src, char **buf); const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); -int virStorageFileChown(virStorageSourcePtr src, uid_t uid, gid_t gid); +int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid); -bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src); +bool virStorageFileSupportsSecurityDriver(const virStorageSource *src); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ce6d21388..3d4bda700 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2082,7 +2082,7 @@ virStorageSourceGetActualType(const virStorageSource *def) bool -virStorageSourceIsLocalStorage(virStorageSourcePtr src) +virStorageSourceIsLocalStorage(const virStorageSource *src) { virStorageType type = virStorageSourceGetActualType(src); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f62244db..bea1c35a2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -349,7 +349,7 @@ int virStorageSourceInitChainElement(virStorageSourcePtr newelem, void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(const virStorageSource *def); -bool virStorageSourceIsLocalStorage(virStorageSourcePtr src); +bool virStorageSourceIsLocalStorage(const virStorageSource *src); bool virStorageSourceIsEmpty(virStorageSourcePtr src); bool virStorageSourceIsBlockLocal(const virStorageSource *src); void virStorageSourceFree(virStorageSourcePtr def);