Prevent unbounded chains by limiting the recursion depth of
virStorageSourceGetMetadataRecurse to the maximum number of image layers
we limit anyways.
This removes the last use of virStorageSourceGetUniqueIdentifier which
will allow us to delete some crusty old infrastructure which isn't
really needed.
Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
---
src/qemu/qemu_domain.c | 4 ++-
src/security/virt-aa-helper.c | 5 +++-
src/storage_file/storage_source.c | 44 +++++++++++++------------------
src/storage_file/storage_source.h | 1 +
tests/virstoragetest.c | 3 ++-
5 files changed, 28 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f818fce271..cf6d41dcad 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7627,7 +7627,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
qemuDomainGetImageIds(cfg, vm, src, disksrc, &uid, &gid);
- if (virStorageSourceGetMetadata(src, uid, gid, report_broken) < 0)
+ if (virStorageSourceGetMetadata(src, uid, gid,
+ QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
+ report_broken) < 0)
return -1;
for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) {
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index c615305320..7e29e43c2e 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -937,9 +937,12 @@ get_files(vahControl * ctl)
continue;
/* XXX - 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.
+ *
+ * The maximum depth is limited to 200 layers similarly to the qemu
+ * implementation.
*/
if (!disk->src->backingStore)
- virStorageSourceGetMetadata(disk->src, -1, -1, false);
+ virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
/* XXX should handle open errors more careful than just ignoring them.
*/
diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c
index ffe150a9b0..19b06b02b8 100644
--- a/src/storage_file/storage_source.c
+++ b/src/storage_file/storage_source.c
@@ -1297,11 +1297,9 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSourcePtr src,
uid_t uid,
gid_t gid,
char **buf,
- size_t *headerLen,
- GHashTable *cycle)
+ size_t *headerLen)
{
int ret = -1;
- const char *uniqueName;
ssize_t len;
if (virStorageSourceInitAs(src, uid, gid) < 0)
@@ -1312,19 +1310,6 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSourcePtr src,
goto cleanup;
}
- if (!(uniqueName = virStorageSourceGetUniqueIdentifier(src)))
- goto cleanup;
-
- if (virHashHasEntry(cycle, uniqueName)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("backing store for %s (%s) is self-referential"),
- NULLSTR(src->path), uniqueName);
- goto cleanup;
- }
-
- if (virHashAddEntry(cycle, uniqueName, NULL) < 0)
- goto cleanup;
-
if ((len = virStorageSourceRead(src, 0, VIR_STORAGE_MAX_HEADER, buf)) < 0)
goto cleanup;
@@ -1343,7 +1328,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
virStorageSourcePtr parent,
uid_t uid, gid_t gid,
bool report_broken,
- GHashTable *cycle,
+ size_t max_depth,
unsigned int depth)
{
virStorageFileFormat orig_format = src->format;
@@ -1352,9 +1337,16 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
g_autofree char *buf = NULL;
g_autoptr(virStorageSource) backingStore = NULL;
- VIR_DEBUG("path=%s format=%d uid=%u gid=%u",
+ VIR_DEBUG("path=%s format=%d uid=%u gid=%u depth=%u",
NULLSTR(src->path), src->format,
- (unsigned int)uid, (unsigned int)gid);
+ (unsigned int)uid, (unsigned int)gid, depth);
+
+ if (depth > max_depth) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("backing store for %s is self-referential or too deeply nested"),
+ NULLSTR(src->path));
+ return -1;
+ }
if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
src->format = VIR_STORAGE_FILE_AUTO;
@@ -1369,7 +1361,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
}
if (virStorageSourceGetMetadataRecurseReadHeader(src, parent, uid, gid,
- &buf, &headerLen, cycle) < 0)
+ &buf, &headerLen) < 0)
return -1;
if (virStorageFileProbeGetMetadata(src, buf, headerLen) < 0)
@@ -1396,7 +1388,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
if ((rv = virStorageSourceGetMetadataRecurse(backingStore, parent,
uid, gid,
report_broken,
- cycle, depth + 1)) < 0) {
+ max_depth, depth + 1)) < 0) {
if (!report_broken)
return 0;
@@ -1427,7 +1419,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
* Extract metadata about the storage volume with the specified
* image format. If image format is VIR_STORAGE_FILE_AUTO, it
* will probe to automatically identify the format. Recurses through
- * the entire chain.
+ * the chain up to @max_depth layers.
*
* Open files using UID and GID (or pass -1 for the current user/group).
* Treat any backing files without explicit type as raw, unless ALLOW_PROBE.
@@ -1445,14 +1437,14 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
int
virStorageSourceGetMetadata(virStorageSourcePtr src,
uid_t uid, gid_t gid,
+ size_t max_depth,
bool report_broken)
{
- g_autoptr(GHashTable) cycle = virHashNew(NULL);
virStorageType actualType = virStorageSourceGetActualType(src);
- VIR_DEBUG("path=%s format=%d uid=%u gid=%u report_broken=%d",
+ VIR_DEBUG("path=%s format=%d uid=%u gid=%u max_depth=%zu report_broken=%d",
src->path, src->format, (unsigned int)uid, (unsigned int)gid,
- report_broken);
+ max_depth, report_broken);
if (src->format <= VIR_STORAGE_FILE_NONE) {
if (actualType == VIR_STORAGE_TYPE_DIR)
@@ -1462,5 +1454,5 @@ virStorageSourceGetMetadata(virStorageSourcePtr src,
}
return virStorageSourceGetMetadataRecurse(src, src, uid, gid,
- report_broken, cycle, 1);
+ report_broken, max_depth, 1);
}
diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h
index 6eb795b301..5e05bde7b1 100644
--- a/src/storage_file/storage_source.h
+++ b/src/storage_file/storage_source.h
@@ -134,6 +134,7 @@ virStorageSourceSupportsBackingChainTraversal(const virStorageSource *src);
int
virStorageSourceGetMetadata(virStorageSourcePtr src,
uid_t uid, gid_t gid,
+ size_t max_depth,
bool report_broken)
ATTRIBUTE_NONNULL(1);
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 84dd813b80..157d577c7d 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -101,7 +101,8 @@ testStorageFileGetMetadata(const char *path,
def->path = g_strdup(path);
- if (virStorageSourceGetMetadata(def, uid, gid, true) < 0)
+ /* 20 is picked as an arbitrary depth, since the chains used here don't exceed it */
+ if (virStorageSourceGetMetadata(def, uid, gid, 20, true) < 0)
return NULL;
return g_steal_pointer(&def);
--
2.29.2
