11:13 a.m.
Few recent changes broke wrong, but apparently widely used
configurations as used didn't really record the image format into the
image.
It turns out we can safely probe the image format in few limited
circumstances which on the other hand should fix the majority of the
problems. Please see patch 8/9 for further explanation.
Peter Krempa (9):
util: storagefile: Drop image format probing by file suffix
virStorageFileGetMetadataRecurse: Remove impossible error report
virStorageFileGetMetadataRecurse: Shuffle around assignment of backing
chain depth
virStorageFileGetMetadataRecurse: Expect NULL src->path
virStorageFileGetMetadataRecurse: Use virHashHasEntry instead of fake
pointers
virStorageFileGetMetadataRecurse: Extract storage access
virStorageFileGetMetadataRecurse: Remove 'cleanup' label
virStorageFileGetMetadataRecurse: Allow format probing under special
circumstances
WIP: Add tool for probing images
src/util/virstoragefile.c | 231 ++++++++++++++++++--------------------
tests/Makefile.am | 13 ++-
tests/qemublockprobe.c | 130 +++++++++++++++++++++
3 files changed, 251 insertions(+), 123 deletions(-)
create mode 100644 tests/qemublockprobe.c
--
2.24.1
11:13 a.m.
New subject: [PATCH 1/9] util: storagefile: Drop image format probing by file suffix
Probing by file suffix was meant to be a last resort if probing by
contents fails or is not supported. For most formats we never specified
any suffix. There's a few formats implementing both magic bytes and
suffix and finally DMG which had only suffix probing. Since suffix
probing is nowhere reliable and only one format depends on in whic has a
comment that qemu doesn't do the probing either drop the whole
infrastructure.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 58 ++++++++++++---------------------------
1 file changed, 17 insertions(+), 41 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 9347c7ab30..4d2e39246c 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -146,12 +146,10 @@ struct FileEncryptionInfo {
int payloadOffset; /* start offset of the volume data (in 512 byte sectors) */
};
-/* Either 'magic' or 'extension' *must* be provided */
struct FileTypeInfo {
int magicOffset; /* Byte offset of the magic */
const char *magic; /* Optional string of file magic
* to check at head of file */
- const char *extension; /* Optional file extension to check */
enum lv_endian endian; /* Endianness of file format */
int versionOffset; /* Byte offset from start of file
@@ -297,17 +295,17 @@ static struct FileEncryptionInfo const qcow2EncryptionInfo[] = {
};
static struct FileTypeInfo const fileTypeInfo[] = {
- [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
+ [VIR_STORAGE_FILE_NONE] = { 0, NULL, LV_LITTLE_ENDIAN,
-1, 0, {0}, 0, 0, 0, NULL, NULL, NULL },
- [VIR_STORAGE_FILE_RAW] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
+ [VIR_STORAGE_FILE_RAW] = { 0, NULL, LV_LITTLE_ENDIAN,
-1, 0, {0}, 0, 0, 0,
luksEncryptionInfo,
NULL, NULL },
- [VIR_STORAGE_FILE_DIR] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
+ [VIR_STORAGE_FILE_DIR] = { 0, NULL, LV_LITTLE_ENDIAN,
-1, 0, {0}, 0, 0, 0, NULL, NULL, NULL },
[VIR_STORAGE_FILE_BOCHS] = {
/*"Bochs Virtual HD Image", */ /* Untested */
- 0, NULL, NULL,
+ 0, NULL,
LV_LITTLE_ENDIAN, 64, 4, {0x20000},
32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL
},
@@ -316,7 +314,7 @@ static struct FileTypeInfo const fileTypeInfo[] = {
#V2.0 Format
modprobe cloop file=$0 && mount -r -t iso9660 /dev/cloop $1
*/ /* Untested */
- 0, NULL, NULL,
+ 0, NULL,
LV_LITTLE_ENDIAN, -1, 0, {0},
-1, 0, 0, NULL, NULL, NULL
},
@@ -324,50 +322,50 @@ static struct FileTypeInfo const fileTypeInfo[] = {
/* XXX QEMU says there's no magic for dmg,
* /usr/share/misc/magic lists double magic (both offsets
* would have to match) but then disables that check. */
- 0, NULL, ".dmg",
+ 0, NULL,
0, -1, 0, {0},
-1, 0, 0, NULL, NULL, NULL
},
[VIR_STORAGE_FILE_ISO] = {
- 32769, "CD001", ".iso",
+ 32769, "CD001",
LV_LITTLE_ENDIAN, -2, 0, {0},
-1, 0, 0, NULL, NULL, NULL
},
[VIR_STORAGE_FILE_VPC] = {
- 0, "conectix", NULL,
+ 0, "conectix",
LV_BIG_ENDIAN, 12, 4, {0x10000},
8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL
},
/* TODO: add getBackingStore function */
[VIR_STORAGE_FILE_VDI] = {
- 64, "\x7f\x10\xda\xbe", ".vdi",
+ 64, "\x7f\x10\xda\xbe",
LV_LITTLE_ENDIAN, 68, 4, {0x00010001},
64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL},
/* Not direct file formats, but used for various drivers */
- [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
+ [VIR_STORAGE_FILE_FAT] = { 0, NULL, LV_LITTLE_ENDIAN,
-1, 0, {0}, 0, 0, 0, NULL, NULL, NULL },
- [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
+ [VIR_STORAGE_FILE_VHD] = { 0, NULL, LV_LITTLE_ENDIAN,
-1, 0, {0}, 0, 0, 0, NULL, NULL, NULL },
- [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", NULL,
LV_LITTLE_ENDIAN,
+ [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", LV_LITTLE_ENDIAN,
-2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0,
PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL },
/* All formats with a backing store probe below here */
[VIR_STORAGE_FILE_COW] = {
- 0, "OOOM", NULL,
+ 0, "OOOM",
LV_BIG_ENDIAN, 4, 4, {2},
4+4+1024+4, 8, 1, NULL, cowGetBackingStore, NULL
},
[VIR_STORAGE_FILE_QCOW] = {
- 0, "QFI", NULL,
+ 0, "QFI",
LV_BIG_ENDIAN, 4, 4, {1},
QCOWX_HDR_IMAGE_SIZE, 8, 1,
qcow1EncryptionInfo,
qcowXGetBackingStore, NULL
},
[VIR_STORAGE_FILE_QCOW2] = {
- 0, "QFI", NULL,
+ 0, "QFI",
LV_BIG_ENDIAN, 4, 4, {2, 3},
QCOWX_HDR_IMAGE_SIZE, 8, 1,
qcow2EncryptionInfo,
@@ -376,12 +374,12 @@ static struct FileTypeInfo const fileTypeInfo[] = {
},
[VIR_STORAGE_FILE_QED] = {
/* https://wiki.qemu.org/Features/QED */
- 0, "QED", NULL,
+ 0, "QED",
LV_LITTLE_ENDIAN, -2, 0, {0},
QED_HDR_IMAGE_SIZE, 8, 1, NULL, qedGetBackingStore, NULL
},
[VIR_STORAGE_FILE_VMDK] = {
- 0, "KDMV", NULL,
+ 0, "KDMV",
LV_LITTLE_ENDIAN, 4, 4, {1, 2, 3},
4+4+4, 8, 512, NULL, vmdk4GetBackingStore, NULL
},
@@ -707,20 +705,6 @@ virStorageFileMatchesMagic(int magicOffset,
}
-static bool
-virStorageFileMatchesExtension(const char *extension,
- const char *path)
-{
- if (extension == NULL)
- return false;
-
- if (virStringHasCaseSuffix(path, extension))
- return true;
-
- return false;
-}
-
-
static bool
virStorageFileMatchesVersion(int versionOffset,
int versionSize,
@@ -842,14 +826,6 @@ virStorageFileProbeFormatFromBuf(const char *path,
"Please report new version to libvir-list(a)redhat.com",
path, virStorageFileFormatTypeToString(possibleFormat));
- /* No magic, so check file extension */
- for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) {
- if (virStorageFileMatchesExtension(fileTypeInfo[i].extension, path)) {
- format = i;
- goto cleanup;
- }
- }
-
cleanup:
VIR_DEBUG("format=%d", format);
return format;
--
2.24.1
9:18 a.m.
New subject: [PATCH 1/9] util: storagefile: Drop image format probing by file suffix
On 2/17/20 11:13 AM, Peter Krempa wrote:
Probing by file suffix was meant to be a last resort if probing by
contents fails or is not supported. For most formats we never specified
any suffix. There's a few formats implementing both magic bytes and
suffix and finally DMG which had only suffix probing. Since suffix
probing is nowhere reliable and only one format depends on in whic has a
which
comment that qemu doesn't do the probing either drop the whole
infrastructure.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 58 ++++++++++++---------------------------
1 file changed, 17 insertions(+), 41 deletions(-)
@@ -324,50 +322,50 @@ static struct FileTypeInfo const fileTypeInfo[]
= {
/* XXX QEMU says there's no magic for dmg,
* /usr/share/misc/magic lists double magic (both offsets
* would have to match) but then disables that check. */
- 0, NULL, ".dmg",
+ 0, NULL,
0, -1, 0, {0},
-1, 0, 0, NULL, NULL, NULL
},
Is it even worth keeping the dmg entry around? But deleting it (if
appropriate) should be separate from this patch.
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
9:37 a.m.
New subject: [PATCH 1/9] util: storagefile: Drop image format probing by file suffix
On Wed, Feb 19, 2020 at 09:18:40 -0600, Eric Blake wrote:
On 2/17/20 11:13 AM, Peter Krempa wrote:
> Probing by file suffix was meant to be a last resort if probing by
> contents fails or is not supported. For most formats we never specified
> any suffix. There's a few formats implementing both magic bytes and
> suffix and finally DMG which had only suffix probing. Since suffix
> probing is nowhere reliable and only one format depends on in whic has a
which
> comment that qemu doesn't do the probing either drop the whole
> infrastructure.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/util/virstoragefile.c | 58 ++++++++++++---------------------------
> 1 file changed, 17 insertions(+), 41 deletions(-)
>
> @@ -324,50 +322,50 @@ static struct FileTypeInfo const fileTypeInfo[] = {
> /* XXX QEMU says there's no magic for dmg,
> * /usr/share/misc/magic lists double magic (both offsets
> * would have to match) but then disables that check. */
> - 0, NULL, ".dmg",
> + 0, NULL,
> 0, -1, 0, {0},
> -1, 0, 0, NULL, NULL, NULL
> },
Is it even worth keeping the dmg entry around? But deleting it (if
appropriate) should be separate from this patch.
The internals currently expect that we have an entry for every image
type supported in libvirt, so we need the dummy entry anyways.
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 2/9] virStorageFileGetMetadataRecurse: Remove impossible error report
We call virStorageFileSupportsBackingChainTraversal which already checks
that the 'storageFileRead' callback is non-NULL, which in turn means
that virStorageFileRead will not return -2.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 4d2e39246c..7aeeab0739 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -5003,15 +5003,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
goto cleanup;
if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER,
- &buf)) < 0) {
- if (headerLen == -2)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("storage file reading is not supported for "
- "storage type %s (protocol: %s)"),
- virStorageTypeToString(src->type),
- virStorageNetProtocolTypeToString(src->protocol));
+ &buf)) < 0)
goto cleanup;
- }
if (virStorageFileGetMetadataInternal(src, buf, headerLen,
&backingFormat) < 0)
--
2.24.1
9:46 a.m.
New subject: [PATCH 2/9] virStorageFileGetMetadataRecurse: Remove impossible error report
On 2/17/20 11:13 AM, Peter Krempa wrote:
We call virStorageFileSupportsBackingChainTraversal which already
checks
that the 'storageFileRead' callback is non-NULL, which in turn means
that virStorageFileRead will not return -2.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
Concur that it was dead code.
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 3/9] virStorageFileGetMetadataRecurse: Shuffle around assignment of backing chain depth
Move the assignment to a place where we know that the backing store is
present rather than having to check in the cleanup section.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7aeeab0739..f8e4102588 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -5053,14 +5053,15 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
ret = 0;
goto cleanup;
}
+
+ backingStore->id = depth;
+ src->backingStore = g_steal_pointer(&backingStore);
} else {
/* add terminator */
- if (!(backingStore = virStorageSourceNew()))
+ if (!(src->backingStore = virStorageSourceNew()))
goto cleanup;
}
- src->backingStore = g_steal_pointer(&backingStore);
-
if (src->externalDataStoreRaw) {
g_autoptr(virStorageSource) externalDataStore = NULL;
@@ -5080,8 +5081,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
ret = 0;
cleanup:
- if (virStorageSourceHasBacking(src))
- src->backingStore->id = depth;
virStorageFileDeinit(src);
return ret;
}
--
2.24.1
9:47 a.m.
New subject: [PATCH 3/9] virStorageFileGetMetadataRecurse: Shuffle around assignment of backing chain depth
On 2/17/20 11:13 AM, Peter Krempa wrote:
Move the assignment to a place where we know that the backing store
is
present rather than having to check in the cleanup section.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 4/9] virStorageFileGetMetadataRecurse: Expect NULL src->path
The path can be NULL e.g. for NBD disks. Use NULLSTR to prevent use of
NULL in %s.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f8e4102588..cf37744d6f 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -4973,7 +4973,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
g_autoptr(virStorageSource) backingStore = NULL;
VIR_DEBUG("path=%s format=%d uid=%u gid=%u",
- src->path, src->format,
+ NULLSTR(src->path), src->format,
(unsigned int)uid, (unsigned int)gid);
/* exit if we can't load information about the current image */
@@ -4995,7 +4995,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if (virHashLookup(cycle, uniqueName)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("backing store for %s (%s) is self-referential"),
- src->path, uniqueName);
+ NULLSTR(src->path), uniqueName);
goto cleanup;
}
--
2.24.1
9:49 a.m.
New subject: [PATCH 4/9] virStorageFileGetMetadataRecurse: Expect NULL src->path
On 2/17/20 11:13 AM, Peter Krempa wrote:
The path can be NULL e.g. for NBD disks. Use NULLSTR to prevent use
of
NULL in %s.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake(a)redhat.com>
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f8e4102588..cf37744d6f 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -4973,7 +4973,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
g_autoptr(virStorageSource) backingStore = NULL;
VIR_DEBUG("path=%s format=%d uid=%u gid=%u",
- src->path, src->format,
+ NULLSTR(src->path), src->format,
(unsigned int)uid, (unsigned int)gid);
Here, it matters in log output;
/* exit if we can't load information about the current image */
@@ -4995,7 +4995,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if (virHashLookup(cycle, uniqueName)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("backing store for %s (%s) is self-referential"),
- src->path, uniqueName);
+ NULLSTR(src->path), uniqueName);
here, we're less likely to see the message. But being consistent is
worthwhile.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 5/9] virStorageFileGetMetadataRecurse: Use virHashHasEntry instead of fake pointers
Replacing virHashLookup by virHashHasEntry allows to use NULL as the
payload of the hash table rather than putting a fake '1' pointer into
the table.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index cf37744d6f..bb3fa65a14 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -4992,14 +4992,14 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if (!(uniqueName = virStorageFileGetUniqueIdentifier(src)))
goto cleanup;
- if (virHashLookup(cycle, uniqueName)) {
+ 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, (void *)1) < 0)
+ if (virHashAddEntry(cycle, uniqueName, NULL) < 0)
goto cleanup;
if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER,
--
2.24.1
9:50 a.m.
New subject: [PATCH 5/9] virStorageFileGetMetadataRecurse: Use virHashHasEntry instead of fake pointers
On 2/17/20 11:13 AM, Peter Krempa wrote:
Replacing virHashLookup by virHashHasEntry allows to use NULL as the
allows us to
payload of the hash table rather than putting a fake '1'
pointer into
the table.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 6/9] virStorageFileGetMetadataRecurse: Extract storage access
Extract the code that directly deals with storage. This allows further
simplification and clarification of virStorageFileGetMetadataRecurse.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 71 ++++++++++++++++++++++++++-------------
1 file changed, 47 insertions(+), 24 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index bb3fa65a14..1e21d44c67 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -4955,31 +4955,18 @@ virStorageFileReportBrokenChain(int errcode,
}
-/* Recursive workhorse for virStorageFileGetMetadata. */
static int
-virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
- virStorageSourcePtr parent,
- uid_t uid, gid_t gid,
- bool report_broken,
- virHashTablePtr cycle,
- unsigned int depth)
+virStorageFileGetMetadataRecurseReadHeader(virStorageSourcePtr src,
+ virStorageSourcePtr parent,
+ uid_t uid,
+ gid_t gid,
+ char **buf,
+ size_t *headerLen,
+ virHashTablePtr cycle)
{
int ret = -1;
const char *uniqueName;
- ssize_t headerLen;
- int backingFormat;
- int rv;
- g_autofree char *buf = NULL;
- g_autoptr(virStorageSource) backingStore = NULL;
-
- VIR_DEBUG("path=%s format=%d uid=%u gid=%u",
- NULLSTR(src->path), src->format,
- (unsigned int)uid, (unsigned int)gid);
-
- /* exit if we can't load information about the current image */
- rv = virStorageFileSupportsBackingChainTraversal(src);
- if (rv <= 0)
- return rv;
+ ssize_t len;
if (virStorageFileInitAs(src, uid, gid) < 0)
return -1;
@@ -5002,10 +4989,47 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if (virHashAddEntry(cycle, uniqueName, NULL) < 0)
goto cleanup;
- if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER,
- &buf)) < 0)
+ if ((len = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER, buf)) < 0)
goto cleanup;
+ *headerLen = len;
+ ret = 0;
+
+ cleanup:
+ virStorageFileDeinit(src);
+ return ret;
+}
+
+
+/* Recursive workhorse for virStorageFileGetMetadata. */
+static int
+virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
+ virStorageSourcePtr parent,
+ uid_t uid, gid_t gid,
+ bool report_broken,
+ virHashTablePtr cycle,
+ unsigned int depth)
+{
+ int ret = -1;
+ size_t headerLen;
+ int backingFormat;
+ int rv;
+ g_autofree char *buf = NULL;
+ g_autoptr(virStorageSource) backingStore = NULL;
+
+ VIR_DEBUG("path=%s format=%d uid=%u gid=%u",
+ NULLSTR(src->path), src->format,
+ (unsigned int)uid, (unsigned int)gid);
+
+ /* exit if we can't load information about the current image */
+ rv = virStorageFileSupportsBackingChainTraversal(src);
+ if (rv <= 0)
+ return rv;
+
+ if (virStorageFileGetMetadataRecurseReadHeader(src, parent, uid, gid,
+ &buf, &headerLen, cycle) <
0)
+ return -1;
+
if (virStorageFileGetMetadataInternal(src, buf, headerLen,
&backingFormat) < 0)
goto cleanup;
@@ -5081,7 +5105,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
ret = 0;
cleanup:
- virStorageFileDeinit(src);
return ret;
}
--
2.24.1
9:52 a.m.
New subject: [PATCH 6/9] virStorageFileGetMetadataRecurse: Extract storage access
On 2/17/20 11:13 AM, Peter Krempa wrote:
Extract the code that directly deals with storage. This allows
further
simplification and clarification of virStorageFileGetMetadataRecurse.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 71 ++++++++++++++++++++++++++-------------
1 file changed, 47 insertions(+), 24 deletions(-)
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 7/9] virStorageFileGetMetadataRecurse: Remove 'cleanup' label
There's nothing to clean up. Make it obvious what is returned.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 50 +++++++++++++++------------------------
1 file changed, 19 insertions(+), 31 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 1e21d44c67..b984204b93 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -5010,7 +5010,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
virHashTablePtr cycle,
unsigned int depth)
{
- int ret = -1;
size_t headerLen;
int backingFormat;
int rv;
@@ -5030,19 +5029,16 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
&buf, &headerLen, cycle) <
0)
return -1;
- if (virStorageFileGetMetadataInternal(src, buf, headerLen,
- &backingFormat) < 0)
- goto cleanup;
+ if (virStorageFileGetMetadataInternal(src, buf, headerLen, &backingFormat) <
0)
+ return -1;
if (src->backingStoreRaw) {
if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
- goto cleanup;
+ return -1;
- if (rv == 1) {
- /* the backing file would not be usable for VM usage */
- ret = 0;
- goto cleanup;
- }
+ /* the backing file would not be usable for VM usage */
+ if (rv == 1)
+ return 0;
backingStore->format = backingFormat;
@@ -5065,17 +5061,14 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if (backingStore->format == VIR_STORAGE_FILE_AUTO_SAFE)
backingStore->format = VIR_STORAGE_FILE_AUTO;
- if ((ret = virStorageFileGetMetadataRecurse(backingStore, parent,
- uid, gid,
- report_broken,
- cycle, depth + 1)) < 0) {
+ if (virStorageFileGetMetadataRecurse(backingStore, parent,
+ uid, gid,
+ report_broken,
+ cycle, depth + 1) < 0) {
if (report_broken)
- goto cleanup;
-
- /* if we fail somewhere midway, just accept and return a
- * broken chain */
- ret = 0;
- goto cleanup;
+ return -1;
+ else
+ return 0;
}
backingStore->id = depth;
@@ -5083,7 +5076,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
} else {
/* add terminator */
if (!(src->backingStore = virStorageSourceNew()))
- goto cleanup;
+ return -1;
}
if (src->externalDataStoreRaw) {
@@ -5091,21 +5084,16 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if ((rv = virStorageSourceNewFromExternalData(src,
&externalDataStore)) < 0)
- goto cleanup;
+ return -1;
- if (rv == 1) {
- /* the file would not be usable for VM usage */
- ret = 0;
- goto cleanup;
- }
+ /* the file would not be usable for VM usage */
+ if (rv == 1)
+ return 0;
src->externalDataStore = g_steal_pointer(&externalDataStore);
}
- ret = 0;
-
- cleanup:
- return ret;
+ return 0;
}
--
2.24.1
9:53 a.m.
New subject: [PATCH 7/9] virStorageFileGetMetadataRecurse: Remove 'cleanup' label
On 2/17/20 11:13 AM, Peter Krempa wrote:
There's nothing to clean up. Make it obvious what is returned.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 50 +++++++++++++++------------------------
1 file changed, 19 insertions(+), 31 deletions(-)
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
Allow format probing to work around lazy clients which did not specify
their format in the overlay. Format probing will be allowed only, if we
are able to probe the image, the probing result was successful and the
probed image does not have any backing or data file.
This relaxes the restrictions which were imposed in commit 3615e8b39bad
in cases when we know that the image probing will not result in security
issues or data corruption.
We perform the image format detection and in the case that we were able
to probe the format and the format does not specify a backing store (or
doesn't support backing store) we can use this format.
With pre-blockdev configurations this will restore the previous
behaviour for the images mentioned above as qemu would probe the format
anyways. It also improves error reporting compared to the old state as
we now report that the backing chain will be broken in case when there
is a backing file.
In blockdev configurations this ensures that libvirt will not cause data
corruption by ending the chain prematurely without notifying the user,
but still allows the old semantics when the users forgot to specify the
format.
The price for this is that libvirt will need to keep the image format
detector still current and working or replace it by invocation of
qemu-img.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 52 ++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index b984204b93..bbdf7be094 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -5010,6 +5010,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
virHashTablePtr cycle,
unsigned int depth)
{
+ virStorageFileFormat orig_format = src->format;
size_t headerLen;
int backingFormat;
int rv;
@@ -5020,10 +5021,17 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
NULLSTR(src->path), src->format,
(unsigned int)uid, (unsigned int)gid);
+ if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
+ src->format = VIR_STORAGE_FILE_AUTO;
+
/* exit if we can't load information about the current image */
rv = virStorageFileSupportsBackingChainTraversal(src);
- if (rv <= 0)
+ if (rv <= 0) {
+ if (orig_format == VIR_STORAGE_FILE_AUTO)
+ return -2;
+
return rv;
+ }
if (virStorageFileGetMetadataRecurseReadHeader(src, parent, uid, gid,
&buf, &headerLen, cycle) <
0)
@@ -5032,6 +5040,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if (virStorageFileGetMetadataInternal(src, buf, headerLen, &backingFormat) <
0)
return -1;
+ /* If we probed the format we MUST ensure that nothing else than the current
+ * image (this includes both backing files and external data store) is
+ * considered for security labelling and/or recursion. */
+ if (orig_format == VIR_STORAGE_FILE_AUTO) {
+ if (src->backingStoreRaw || src->externalDataStoreRaw) {
+ src->format = VIR_STORAGE_FILE_RAW;
+ VIR_FREE(src->backingStoreRaw);
+ VIR_FREE(src->externalDataStoreRaw);
+ return -2;
+ }
+ }
+
if (src->backingStoreRaw) {
if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
return -1;
@@ -5042,33 +5062,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
backingStore->format = backingFormat;
- if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
- /* Assuming the backing store to be raw can lead to failures. We do
- * it only when we must not report an error to prevent losing VMs.
- * Otherwise report an error.
- */
- if (report_broken) {
+ if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
+ uid, gid,
+ report_broken,
+ cycle, depth + 1)) < 0) {
+ if (!report_broken)
+ return 0;
+
+ if (rv == -2) {
virReportError(VIR_ERR_OPERATION_INVALID,
_("format of backing image '%s' of image
'%s' was not specified in the image metadata "
"(See https://libvirt.org/kbase/backing_chains.html
for troubleshooting)"),
src->backingStoreRaw, NULLSTR(src->path));
- return -1;
}
- backingStore->format = VIR_STORAGE_FILE_RAW;
- }
-
- if (backingStore->format == VIR_STORAGE_FILE_AUTO_SAFE)
- backingStore->format = VIR_STORAGE_FILE_AUTO;
-
- if (virStorageFileGetMetadataRecurse(backingStore, parent,
- uid, gid,
- report_broken,
- cycle, depth + 1) < 0) {
- if (report_broken)
- return -1;
- else
- return 0;
+ return -1;
}
backingStore->id = depth;
--
2.24.1
10:21 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On 2/17/20 11:13 AM, Peter Krempa wrote:
Allow format probing to work around lazy clients which did not
specify
their format in the overlay. Format probing will be allowed only, if we
s/only, if/only if/
are able to probe the image, the probing result was successful and
the
probed image does not have any backing or data file.
This relaxes the restrictions which were imposed in commit 3615e8b39bad
in cases when we know that the image probing will not result in security
issues or data corruption.
It took me a few minutes of thinking about this.
Scenario 1:
base.raw <- wrap.qcow2
where wrap.qcow2 specifies backing of base.raw but not the format. If
we probe, we can have a couple of outcomes:
1a: base.raw probes as raw (the probed image has no backing or data
file), using it as raw is safe (it matches what wrap.qcow2 should have
specified but didn't, and we aren't changing the data the guest would
read nor are we opening unexpected files)
1b: base.raw probes as qcow2 (because of whatever the guest wrote
there), using it as qcow2 is wrong - the guest will see corrupted data.
What's more, if the probe sees it as qcow2 with backing file, and we
open the backing file, it also has security implications.
Scenario 2:
base.qcow2 <- wrap.qcow2
where wrap.qcow2 specifies backing of base.qcow2 but not the format. If
we probe, we will always have just one outcome:
2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if
base.qcow2 has a further backing image, the backing chain is now
dependent on a probe.
Since 1b and 2a have the same probe result, but massively different data
corruption and/or security concerns, it is NOT sufficient to claim that
a probe was safe merely because "the probed image does not have any
backing or data file". It is ONLY safe if the probe turns up raw. Any
other probed format is inherently unsafe.
We perform the image format detection and in the case that we were able
to probe the format and the format does not specify a backing store (or
doesn't support backing store) we can use this format.
Wrong. The condition needs to be:
If we probe the format, and the probe returns raw, then it is safe to
use raw as the format.
With pre-blockdev configurations this will restore the previous
behaviour for the images mentioned above as qemu would probe the format
anyways. It also improves error reporting compared to the old state as
we now report that the backing chain will be broken in case when there
is a backing file.
Improved error reporting because the probe returned qcow2 that would
have required us to chase a backing file is fine; but while blindly
accepting a qcow2 probe result when there is no backing file might avoid
the security issue of chasing a backing file under guest control, it
does not solve the data corruption issue of interpreting data as qcow2
that should have been interpreted as raw.
In blockdev configurations this ensures that libvirt will not cause data
corruption by ending the chain prematurely without notifying the user,
but still allows the old semantics when the users forgot to specify the
format.
The only time where it is safe to imply a forgotten format is if the
probed format is still raw.
The price for this is that libvirt will need to keep the image format
detector still current and working or replace it by invocation of
qemu-img.
Maybe. Any format that qemu recognizes but libvirt does not risks a
case where libvirt probes the image as raw but lets qemu re-probe the
image and then qemu exposes different data. But as long as libvirt
always passes explicit format to qemu (including explicit raw format of
a backing file whose format was forgotten but probing said it was raw),
then it doesn't matter what other formats libvirt can probe for. The
only benefit for libvirt probing formats is then for better error
messages for non-raw.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 52 ++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index b984204b93..bbdf7be094 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -5010,6 +5010,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
virHashTablePtr cycle,
unsigned int depth)
{
+ virStorageFileFormat orig_format = src->format;
size_t headerLen;
int backingFormat;
int rv;
@@ -5020,10 +5021,17 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
NULLSTR(src->path), src->format,
(unsigned int)uid, (unsigned int)gid);
+ if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
+ src->format = VIR_STORAGE_FILE_AUTO;
+
/* exit if we can't load information about the current image */
rv = virStorageFileSupportsBackingChainTraversal(src);
- if (rv <= 0)
+ if (rv <= 0) {
+ if (orig_format == VIR_STORAGE_FILE_AUTO)
+ return -2;
+
return rv;
+ }
if (virStorageFileGetMetadataRecurseReadHeader(src, parent, uid, gid,
&buf, &headerLen, cycle)
< 0)
@@ -5032,6 +5040,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if (virStorageFileGetMetadataInternal(src, buf, headerLen, &backingFormat) <
0)
return -1;
+ /* If we probed the format we MUST ensure that nothing else than the current
+ * image (this includes both backing files and external data store) is
+ * considered for security labelling and/or recursion. */
Grammar:
If we probed the format, we MUST ensure that nothing besides the current
image (including both backing files and external data store) will be
considered for security labelling and/or recursion.
+ if (orig_format == VIR_STORAGE_FILE_AUTO) {
+ if (src->backingStoreRaw || src->externalDataStoreRaw) {
+ src->format = VIR_STORAGE_FILE_RAW;
+ VIR_FREE(src->backingStoreRaw);
+ VIR_FREE(src->externalDataStoreRaw);
+ return -2;
+ }
+ }
+
if (src->backingStoreRaw) {
if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
return -1;
@@ -5042,33 +5062,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
backingStore->format = backingFormat;
- if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
- /* Assuming the backing store to be raw can lead to failures. We do
- * it only when we must not report an error to prevent losing VMs.
- * Otherwise report an error.
- */
- if (report_broken) {
+ if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
+ uid, gid,
+ report_broken,
+ cycle, depth + 1)) < 0) {
+ if (!report_broken)
+ return 0;
+
+ if (rv == -2) {
virReportError(VIR_ERR_OPERATION_INVALID,
_("format of backing image '%s' of image
'%s' was not specified in the image metadata "
"(See
https://libvirt.org/kbase/backing_chains.html for troubleshooting)"),
src->backingStoreRaw, NULLSTR(src->path));
I disagree with the logic here. What we really need is:
If the backing format was not specified, we probe to see what is there.
If the result of that probe is raw, it is safe to treat the image as
raw. If the result is anything else, we must report an error stating
that what we probed could not be trusted unless the user adds an
explicit backing format (either confirming that our probe was correct,
or with the correct format overriding what we mis-probed).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:40 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Wed, Feb 19, 2020 at 10:21:00 -0600, Eric Blake wrote:
On 2/17/20 11:13 AM, Peter Krempa wrote:
> Allow format probing to work around lazy clients which did not specify
> their format in the overlay. Format probing will be allowed only, if we
s/only, if/only if/
> are able to probe the image, the probing result was successful and the
> probed image does not have any backing or data file.
>
> This relaxes the restrictions which were imposed in commit 3615e8b39bad
> in cases when we know that the image probing will not result in security
> issues or data corruption.
It took me a few minutes of thinking about this.
Scenario 1:
base.raw <- wrap.qcow2
where wrap.qcow2 specifies backing of base.raw but not the format. If we
probe, we can have a couple of outcomes:
1a: base.raw probes as raw (the probed image has no backing or data file),
using it as raw is safe (it matches what wrap.qcow2 should have specified
but didn't, and we aren't changing the data the guest would read nor are we
opening unexpected files)
1b: base.raw probes as qcow2 (because of whatever the guest wrote there),
using it as qcow2 is wrong - the guest will see corrupted data. What's more,
if the probe sees it as qcow2 with backing file, and we open the backing
file, it also has security implications.
We don't open the backing file in the proposed logic. That is
specifically forbidden.
Also in pre-blockdev configurations the same situation would happen as
we allowed qemu to probe the backing file despite assuming the format to
be raw. This was as we weren't able to tell qemu what the backing format
is. This is fixed with -blockdev, so this scenario is exactly the same
as it was before.
Scenario 2:
base.qcow2 <- wrap.qcow2
where wrap.qcow2 specifies backing of base.qcow2 but not the format. If we
probe, we will always have just one outcome:
2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if
base.qcow2 has a further backing image, the backing chain is now dependent
on a probe.
Since 1b and 2a have the same probe result, but massively different data
corruption and/or security concerns, it is NOT sufficient to claim that a
probe was safe merely because "the probed image does not have any backing or
data file". It is ONLY safe if the probe turns up raw. Any other probed
format is inherently unsafe.
I disagree here. If the probe of 'base.qcow2' showed a backing file, we
refuse startup right away. If it didn't show any backing file, we
continue:
1) with old (pre-blockdev qemus) libvirt starts qemu with wrap.qcow2 as
image. Qemu tries to open the backing file and probes it. Now if we've
mis-detected that there is a backing file, we will depend on sVirt to
save us. This scenario is how all of this was working until 2 months
ago. It's because we've asumed that the format of 'base.qcow2' was raw,
but started qemu. Since we didn't tell qemu what the format of
'base.qcow2' as it was impossible, we've relied on sVirt anyways.
This is the same as it was before.
2) with new qemu, we do the same, but start qemu and specifically tell
it that "the backing format is qcow2 and that the image has no backing
file. That way qemu doesn't even attempt to open anythting. This means
that this scenario fixes any deployment without selinux, while keeping
old semantics around.
> We perform the image format detection and in the case that we
were able
> to probe the format and the format does not specify a backing store (or
> doesn't support backing store) we can use this format.
Wrong. The condition needs to be:
If we probe the format, and the probe returns raw, then it is safe to use
raw as the format.
That doesn't solve anythign then. The idea of this series is to relax
the restrictions we've imposed after introducing blockdev to return the
main semantics back to what we've allowed in pre-blockdev
configurations.
Namely a user creates an overlay on top of single raw/qcow2 image and
expects it to work. And it's not just random users, libguestfs and
openstack also neglected to set the backing format.
> With pre-blockdev configurations this will restore the previous
> behaviour for the images mentioned above as qemu would probe the format
> anyways. It also improves error reporting compared to the old state as
> we now report that the backing chain will be broken in case when there
> is a backing file.
Improved error reporting because the probe returned qcow2 that would have
required us to chase a backing file is fine; but while blindly accepting a
qcow2 probe result when there is no backing file might avoid the security
issue of chasing a backing file under guest control, it does not solve the
data corruption issue of interpreting data as qcow2 that should have been
interpreted as raw.
Again, this same scenario would happen previously, where we allowed qemu
to probe despite assuming the image to be raw, since we were unable to
tell qemu otherwise.
> In blockdev configurations this ensures that libvirt will not
cause data
> corruption by ending the chain prematurely without notifying the user,
> but still allows the old semantics when the users forgot to specify the
> format.
The only time where it is safe to imply a forgotten format is if the probed
format is still raw.
>
> The price for this is that libvirt will need to keep the image format
> detector still current and working or replace it by invocation of
> qemu-img.
Maybe. Any format that qemu recognizes but libvirt does not risks a case
where libvirt probes the image as raw but lets qemu re-probe the image and
That doesn't happen with blockdev. We dont' let qemu probe.
then qemu exposes different data. But as long as libvirt always
passes
explicit format to qemu (including explicit raw format of a backing file
whose format was forgotten but probing said it was raw), then it doesn't
matter what other formats libvirt can probe for. The only benefit for
libvirt probing formats is then for better error messages for non-raw.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/util/virstoragefile.c | 52 ++++++++++++++++++++++-----------------
> 1 file changed, 30 insertions(+), 22 deletions(-)
>
[....]
> - if (backingStore->format == VIR_STORAGE_FILE_AUTO)
{
> - /* Assuming the backing store to be raw can lead to failures. We do
> - * it only when we must not report an error to prevent losing VMs.
> - * Otherwise report an error.
> - */
> - if (report_broken) {
> + if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
> + uid, gid,
> + report_broken,
> + cycle, depth + 1)) < 0) {
> + if (!report_broken)
> + return 0;
> +
> + if (rv == -2) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> _("format of backing image '%s' of
image '%s' was not specified in the image metadata "
> "(See
https://libvirt.org/kbase/backing_chains.html for troubleshooting)"),
> src->backingStoreRaw, NULLSTR(src->path));
I disagree with the logic here. What we really need is:
If the backing format was not specified, we probe to see what is there. If
the result of that probe is raw, it is safe to treat the image as raw. If
the result is anything else, we must report an error stating that what we
probed could not be trusted unless the user adds an explicit backing format
(either confirming that our probe was correct, or with the correct format
overriding what we mis-probed).
As noted above, using this logic would be pointless. We are better off
just reporting the error always if we also don't allow qcow2 without
backing file to be used as it was previously used.
10:59 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Wed, Feb 19, 2020 at 17:40:34 +0100, Peter Krempa wrote:
On Wed, Feb 19, 2020 at 10:21:00 -0600, Eric Blake wrote:
> On 2/17/20 11:13 AM, Peter Krempa wrote:
[...]
> > With pre-blockdev configurations this will restore the
previous
> > behaviour for the images mentioned above as qemu would probe the format
> > anyways. It also improves error reporting compared to the old state as
> > we now report that the backing chain will be broken in case when there
> > is a backing file.
>
> Improved error reporting because the probe returned qcow2 that would have
> required us to chase a backing file is fine; but while blindly accepting a
> qcow2 probe result when there is no backing file might avoid the security
> issue of chasing a backing file under guest control, it does not solve the
As said, we allowed that before and it's fixed with blockdev.
> data corruption issue of interpreting data as qcow2 that should
have been
> interpreted as raw.
Also don't forget that there's the issue of mis-detecting qcow2 as raw.
That results in worse as the other way around as qemu will object if an
image is not really qcow2 if being told that it is. If we declare it as
raw, the guests starts and sees garbage. [1]
This means that if we don't want to allow probing of 'qcow2 without
backing' as working in the above scenario, we are better of just always
requiring users to pass the format in the overlay as It's not worth just
doing it for 'raw' images with all the potential drawbacks of
mis-detecting qcow2 as raw.
[1] That's what happened when blockdev was introduced and it resulted in
the logic which I'm wanting to relax. We didn't refuse it before.
11:07 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On 2/19/20 10:40 AM, Peter Krempa wrote:
> 1b: base.raw probes as qcow2 (because of whatever the guest wrote
there),
> using it as qcow2 is wrong - the guest will see corrupted data. What's more,
> if the probe sees it as qcow2 with backing file, and we open the backing
> file, it also has security implications.
We don't open the backing file in the proposed logic. That is
specifically forbidden.
I agree that you contained the security risk by not following any file
names mentioned in based.raw when interpreted as qcow2, but that does
not mean you contained the data corruption risk.
Also in pre-blockdev configurations the same situation would happen as
we allowed qemu to probe the backing file despite assuming the format to
be raw. This was as we weren't able to tell qemu what the backing format
is. This is fixed with -blockdev, so this scenario is exactly the same
as it was before.
We assumed it to be raw, the question is whether qemu also assumed it to
be raw, or qemu assumed it to be qcow2. If qemu assumed it to be qcow2,
sVirt may have saved us from the assumption going haywire and opening
forbidden files, but it did NOT save the guest from seeing corrupted
data. If qemu assumed it to be raw, then all that was missing
pre-blockdev was a way for us to tell qemu its assumptions were correct.
>
> Scenario 2:
>
> base.qcow2 <- wrap.qcow2
>
> where wrap.qcow2 specifies backing of base.qcow2 but not the format. If we
> probe, we will always have just one outcome:
>
> 2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if
> base.qcow2 has a further backing image, the backing chain is now dependent
> on a probe.
>
> Since 1b and 2a have the same probe result, but massively different data
> corruption and/or security concerns, it is NOT sufficient to claim that a
> probe was safe merely because "the probed image does not have any backing or
> data file". It is ONLY safe if the probe turns up raw. Any other probed
> format is inherently unsafe.
I disagree here. If the probe of 'base.qcow2' showed a backing file, we
refuse startup right away. If it didn't show any backing file, we
continue:
1) with old (pre-blockdev qemus) libvirt starts qemu with wrap.qcow2 as
image. Qemu tries to open the backing file and probes it. Now if we've
mis-detected that there is a backing file, we will depend on sVirt to
save us. This scenario is how all of this was working until 2 months
ago. It's because we've asumed that the format of 'base.qcow2' was raw,
but started qemu. Since we didn't tell qemu what the format of
'base.qcow2' as it was impossible, we've relied on sVirt anyways.
This is the same as it was before.
So you're arguing that because qemu probes and treats the file as qcow2,
even though we had assumed raw, but that the data actually was qcow2 (so
the guest did not see data corruption), that we want to continue this
practice by explicitly telling qemu that it is qcow2.
Then this all boils down to "what does qemu do when it probes an image
that does not result in raw". If qemu trusted the probe results anyway,
then data corruption was already possible, but once the data corruption
happens, the guest can no longer reverse the corruption nor cause
further security damage (once qemu treats a previously-raw image as
qcow2, the guest can no longer rewrite the qcow2 metadata). If qemu
failed to use the image because it treated the image as raw, then
libvirt's decision to tell qemu that the image is qcow2 will CAUSE data
corruption.
I recall that older qemu did blindly trust the probe results, but that
there was discussion on the qemu list about patching things to warn
about probes that resulted in anything other than raw. But I could not
quickly find mailing list discussions or specific patches that mention
what actually happens; the closest I got was:
commit e4c8f2925d22584b2008aadea5c70e1e05c2a522
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Tue Nov 20 17:56:46 2018 +0000
iotests: fix nbd test 233 to work correctly with raw images
The first qemu-io command must honour the $IMGFMT that is set rather
than hardcoding qcow2. The qemu-nbd commands should also set $IMGFMT
to avoid the insecure format probe warning.
2) with new qemu, we do the same, but start qemu and specifically tell
it that "the backing format is qcow2 and that the image has no backing
file. That way qemu doesn't even attempt to open anythting. This means
that this scenario fixes any deployment without selinux, while keeping
old semantics around.
If you tell qemu that something is qcow2, but qemu has ever in the past
treated that file as raw, then you are forcing data corruption on the
guest, even if you avoid a security issue of chasing further backing
files from treating that image as qcow2.
>> We perform the image format detection and in the case that we were able
>> to probe the format and the format does not specify a backing store (or
>> doesn't support backing store) we can use this format.
>
> Wrong. The condition needs to be:
>
> If we probe the format, and the probe returns raw, then it is safe to use
> raw as the format.
That doesn't solve anythign then. The idea of this series is to relax
the restrictions we've imposed after introducing blockdev to return the
main semantics back to what we've allowed in pre-blockdev
configurations.
The only way I see that might be safe to relax restrictions is if the
filename heuristics give us a clue as to the user's intent. Data
corruption is bad enough that we should never be the cause of it. A
user with a broken setup deserves a decent error message that their
setup is broken, and how to fix it (even so much as "we detected qcow2,
but weren't sure if you might have had a raw file instead, so do this to
tell us which one you meant"), but blindly picking one always creates a
corner case where our choice is wrong.
Namely a user creates an overlay on top of single raw/qcow2 image and
expects it to work. And it's not just random users, libguestfs and
openstack also neglected to set the backing format.
Yes, and they are getting patched. Belatedly, but better late than never.
>> The price for this is that libvirt will need to keep the
image format
>> detector still current and working or replace it by invocation of
>> qemu-img.
>
> Maybe. Any format that qemu recognizes but libvirt does not risks a case
> where libvirt probes the image as raw but lets qemu re-probe the image and
That doesn't happen with blockdev. We dont' let qemu probe.
We are just shifting the burden on who causes the data corruption when a
probe goes wrong - it used to be qemu, now it is libvirt.
>
> I disagree with the logic here. What we really need is:
>
> If the backing format was not specified, we probe to see what is there. If
> the result of that probe is raw, it is safe to treat the image as raw. If
> the result is anything else, we must report an error stating that what we
> probed could not be trusted unless the user adds an explicit backing format
> (either confirming that our probe was correct, or with the correct format
> overriding what we mis-probed).
As noted above, using this logic would be pointless. We are better off
just reporting the error always if we also don't allow qcow2 without
backing file to be used as it was previously used.
Then report the error always.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
12:57 p.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Wed, Feb 19, 2020 at 11:07:09 -0600, Eric Blake wrote:
On 2/19/20 10:40 AM, Peter Krempa wrote:
[....]
> Namely a user creates an overlay on top of single raw/qcow2
image and
> expects it to work. And it's not just random users, libguestfs and
> openstack also neglected to set the backing format.
>
Yes, and they are getting patched. Belatedly, but better late than never.
In that case, qemu-img should also be fixed to forbid 'create' without
-F. Otherwise it's hard to argue that it's a wrong thing to do.
> > > The price for this is that libvirt will need to keep
the image format
> > > detector still current and working or replace it by invocation of
> > > qemu-img.
> >
> > Maybe. Any format that qemu recognizes but libvirt does not risks a case
> > where libvirt probes the image as raw but lets qemu re-probe the image and
>
> That doesn't happen with blockdev. We dont' let qemu probe.
We are just shifting the burden on who causes the data corruption when a
probe goes wrong - it used to be qemu, now it is libvirt.
> >
> > I disagree with the logic here. What we really need is:
> >
> > If the backing format was not specified, we probe to see what is there. If
> > the result of that probe is raw, it is safe to treat the image as raw. If
> > the result is anything else, we must report an error stating that what we
> > probed could not be trusted unless the user adds an explicit backing format
> > (either confirming that our probe was correct, or with the correct format
> > overriding what we mis-probed).
>
> As noted above, using this logic would be pointless. We are better off
> just reporting the error always if we also don't allow qcow2 without
> backing file to be used as it was previously used.
Then report the error always.
Well that's what we do right now. It seems kind of tempting to call this
a right thing but let me summarize the semantics:
- The "true" detection cases are not problematic
- advantage is that existing (arguably suboptimal) configurations
will keep working if we detect
- For the "false" detection cases:
- misdetection does not breach security (given that sVirt is used)
- data corruption can occur if libvirt detects something else than
qemu
- this is true both directions (qcow2->raw, raw->qcow2)
and the tradeoff:
1) If we allow detection, we trade compatibility for the (small)
possibility of having to deal with corruption.
2) If we disallow detection we trade regression of behaviour for data
corruption not being our problem.
I started this trhead because I feel that the value of 1) is more than
2). Especially short term since qemu-img's default behaviour is allowing
creation of images which would break with libvirt and the fact that
we've tolerated the wrong behaviour for years.
Additionally I think that we could just get rid of the copy of the image
detection copy in libvirt and replace it by invocation of qemu-img. That
way we could avoid any discrepancies in the detection process in the
first place.
1:12 p.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
[adding qemu]
On 2/19/20 12:57 PM, Peter Krempa wrote:
>> Namely a user creates an overlay on top of single raw/qcow2
image and
>> expects it to work. And it's not just random users, libguestfs and
>> openstack also neglected to set the backing format.
>>
>
> Yes, and they are getting patched. Belatedly, but better late than never.
In that case, qemu-img should also be fixed to forbid 'create' without
-F. Otherwise it's hard to argue that it's a wrong thing to do.
Allowing -b without -F when the backing file probes as raw is safe, but
yes, I agree qemu-img create should start a deprecation period of
warning if -F is omitted, and turn it into a hard error when enough time
elapses.
>>>> The price for this is that libvirt will need to keep the image format
>>>> detector still current and working or replace it by invocation of
>>>> qemu-img.
>>>
>>> Maybe. Any format that qemu recognizes but libvirt does not risks a case
>>> where libvirt probes the image as raw but lets qemu re-probe the image and
>>
>> That doesn't happen with blockdev. We dont' let qemu probe.
>
> We are just shifting the burden on who causes the data corruption when a
> probe goes wrong - it used to be qemu, now it is libvirt.
>
>
>>>
>>> I disagree with the logic here. What we really need is:
>>>
>>> If the backing format was not specified, we probe to see what is there. If
>>> the result of that probe is raw, it is safe to treat the image as raw. If
>>> the result is anything else, we must report an error stating that what we
>>> probed could not be trusted unless the user adds an explicit backing format
>>> (either confirming that our probe was correct, or with the correct format
>>> overriding what we mis-probed).
>>
>> As noted above, using this logic would be pointless. We are better off
>> just reporting the error always if we also don't allow qcow2 without
>> backing file to be used as it was previously used.
>
> Then report the error always.
Well that's what we do right now. It seems kind of tempting to call this
a right thing but let me summarize the semantics:
- The "true" detection cases are not problematic
- advantage is that existing (arguably suboptimal) configurations
will keep working if we detect
- For the "false" detection cases:
- misdetection does not breach security (given that sVirt is used)
- data corruption can occur if libvirt detects something else than
qemu
- this is true both directions (qcow2->raw, raw->qcow2)
and the tradeoff:
1) If we allow detection, we trade compatibility for the (small)
possibility of having to deal with corruption.
2) If we disallow detection we trade regression of behaviour for data
corruption not being our problem.
I started this trhead because I feel that the value of 1) is more than
2). Especially short term since qemu-img's default behaviour is allowing
creation of images which would break with libvirt and the fact that
we've tolerated the wrong behaviour for years.
Additionally I think that we could just get rid of the copy of the image
detection copy in libvirt and replace it by invocation of qemu-img. That
way we could avoid any discrepancies in the detection process in the
first place.
Now there's an interesting thought. Since data corruption occurs when
there is disagreement about which mode to use, getting libvirt out of
the probing business by deferring all decisions to qemu-img info is a
smart move - if qemu says an image probes as qcow2 (in an environment
where probing is safe), then libvirt passing an explicit qcow2 to qemu
for guest usage (in an environment where probing is not safe) will at
least see the same guest-visible data. Less code to maintain in
libvirt, and no chance for a mismatch between the two projects on which
format a probe should result in.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
7:34 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
[adding qemu]
Adding Daniel as he objected to qemu-img.
On 2/19/20 12:57 PM, Peter Krempa wrote:
[...]
> Additionally I think that we could just get rid of the copy of
the image
> detection copy in libvirt and replace it by invocation of qemu-img. That
> way we could avoid any discrepancies in the detection process in the
> first place.
Now there's an interesting thought. Since data corruption occurs when there
is disagreement about which mode to use, getting libvirt out of the probing
business by deferring all decisions to qemu-img info is a smart move - if
qemu says an image probes as qcow2 (in an environment where probing is
safe), then libvirt passing an explicit qcow2 to qemu for guest usage (in an
environment where probing is not safe) will at least see the same
guest-visible data. Less code to maintain in libvirt, and no chance for a
mismatch between the two projects on which format a probe should result in.
I raised the use of qemu-img to Daniel and he disagreed with use of
qemu-img in libvirt for doing the probing on multiple reasons:
- qemu-img instantiates many data structures relevant to the format so
it has a huge attack surface
- performance of spawning extra processes would be way worse
While at least from the point of view of VM startup both can be
challenged this adds a complete new orthogonal dimension to the problem
I'm attempting to fix.
I'll reiterate the historical state of the problem because I think it's
important:
Pre-blockdev:
- we internally assumed that if the image format of an backing image
was not present in the overlay it is 'raw'
- this influenced security labelling but not actually how qemu viewed
or probed the file. If it was qcow2 probed as qcow2 qemu opened it
as qcow2 possibly even including the backing file if selinux or
other mechanism didn't prevent it.
post-blockdev:
- the assumption of 'raw' would now be expressed into the qemu
configuration. This assumption turned into data corruption since we
no longer allowed qemu to probe the format and forced it as raw.
- fix was to always require the format to be recorded in the overlay
- this made users unhappy who neglected to record the format into the
overlay when creating it manually
Now since qemu didn't discourage the creation of overlays without format
there still are many users which will inevitably hit this problem when
used with libvirt.
My proposal tries to mitigate the regressions in behaviour in the valid
and secure use cases. (If the image whose format we detect doesn't have
a backing image)
This comes at a trade-off though. As Eric pointed out, if the format
probed by libvirt's internal code disagrees with qemu's format we are
getting into the image corruption region.
As a mitigation to the above I suggested using qemu-img to probe but
that's a complex change and as mentioned above not really welcome
upstream.
Now this adds an interresting dimension to this problem. If libvirt
forces the users to specify the image format, and the users don't know
it they will probe. So we are basically making this a problem of
somebody else. [2] As you can see in that patch, it uses 'qemu-img'
anyways and also additionally actually allows the chain to continue
deeper! [3]
A partial relief to the image detection problem is that qemu would
refuse to start if an non-qcow2 image is used in qcow2, thus we really
only run into problems if qcow2 is mis-detected as raw.
This boils down to whether we want to accept some possibility of image
corruption in trade for avoiding regression of behaviour in the secure
cases as well as management apps and users not having to re-invent when
probing an image is actually safe.
Finally I think we should either decide to fix it in this release, or
stick with the error message forever. Fixing it later will not make
much sense as many users already fixed their scripts and we'd just add
back the trade-off of possible image corruption.
Peter
[1] If e.g. the security subsystem of the host didn't forbid the use of
the backing file such a qcow2 qemu would happily open it.
[2] https://www.redhat.com/archives/libguestfs/2020-February/msg00013.html
[3] As implemented in [2] the backing image is not checked whether it
has a backing file or not but the format is probed, which way result
into accessing the backing chain of the probed image.
Prior to this detection, it would be prevented by sVirt or alternatively
also by libvit itself in -blockdev mode when this patch would be
accepted.
8:24 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:
On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
> [adding qemu]
Adding Daniel as he objected to qemu-img.
>
> On 2/19/20 12:57 PM, Peter Krempa wrote:
[...]
> > Additionally I think that we could just get rid of the copy of the image
> > detection copy in libvirt and replace it by invocation of qemu-img. That
> > way we could avoid any discrepancies in the detection process in the
> > first place.
>
> Now there's an interesting thought. Since data corruption occurs when there
> is disagreement about which mode to use, getting libvirt out of the probing
> business by deferring all decisions to qemu-img info is a smart move - if
> qemu says an image probes as qcow2 (in an environment where probing is
> safe), then libvirt passing an explicit qcow2 to qemu for guest usage (in an
> environment where probing is not safe) will at least see the same
> guest-visible data. Less code to maintain in libvirt, and no chance for a
> mismatch between the two projects on which format a probe should result in.
I raised the use of qemu-img to Daniel and he disagreed with use of
qemu-img in libvirt for doing the probing on multiple reasons:
- qemu-img instantiates many data structures relevant to the format so
it has a huge attack surface
This was the most critical reason why we have this code in libvirt
in the first place.
NB, we need to be sure we are comparing the same things between
libvirt and QEMU when we discuss "probing".
What we're talking about by probing in libvirt context is
- Detect the image format
- Detect the image virtual size
- Detect the image physical size
- Detect the image backing store location
- Detect the image backing store format
- Detect the image encryption usage
In QEMU 'format probing' (as impl by bdrv_probe in QEMU's block layer)
only covers the very first point, 'detect the image format'. All the
other information can only be acquired by opening the image (bdrv_open
in QEMU's block layer).
The issue is that bdrv_open does waaaaaay more than we desire here,
because it is serving the broader purpose of allowing QEMU to actually
use the image. qcow2 is probably the worst case example, as it has
to parse the image and setup data structures for l1, l2 tables,
refcount tables, snapshots, and initialize the encryption layer if
present.
It is known that this code is vulnerable to maliciously created
qcow2 images. This resulted in OpenStack being vulnerable to
CVE-2015-5162 https://bugs.launchpad.net/ossa/+bug/1449062
It isn't possible to do anything to avoid this risk if you are
invoking qemu-img on untrustworthy images. The best you can do
is to mitigate the effects by placing memory/CPU ulimits on
the qemu-img process. Determining these limits then introduces
a new problem, as you have to pick a limit which is low enough
to avoid DoS, while large enough to allow all valid usage.
Since mitigating CVE-2015-5162 OpenStack has faced this problem
with users reporting that the limits it set were breaking valid
usage, as so had to increase the limits, which increases the
DoS impact. Then there's also the pain that OSP suffered when
QEMU introduced mandatory locking which broke all existing
usage of 'qemu-img info' when a VM was running.
Of course when you launch QEMU later, it is susceptible to the
DoS in the system emulator, but this is mitigated by fact that
upfront probing is going to reject some malicious images. If
some bad images do get past, then it will be dealt with by the
mgmt apps normal monitoring of a running VM resource usage
and/or cgroups limits.
The libvirt probing code is designed to do the minimal work
needed to get the information we require. Of course there may
be bugs in libvirt's code, but it is much more straightforward
for us to analyse & understand risks, as most of the problematic
code that QEMU has simply doesn't exist in libvirt.
- performance of spawning extra processes would be way worse
Yes, this was the second motivation for having this code in libvirt
originally. The QEMU VM startup case wasn't the target, but rather
storage pools code. When we start a storage pool with 100's of images,
the time to spawn 100's of instances of qemu-img adds up very quickly.
Even if qemu-img had exactly the same minimalist code as libvirt's
current probing logic it would still be worse. The overhead of process
startup vs the time spent probing the image is a poor ratio, such that
process startup/exec time dominates.
The third reason why libvirt has this code is because historically
the error reporting from qemu-img has been quite unhelpful - many
errors just end up being a generic EINVAL error message. Things have
improved over time, but error reporting is always a challenge when
spawning external commands to do work.
The fourth reason why libvirt has this image file detection code is
that it is used by non-QEMU drivers in libvirt, mostly notably the
storage pool driver, and we didn't wish to force people to install
qemu-img on hosts which were not running the QEMU virt driver.
I don't think this reason is especially a technical show stopper,
since these days all distros allow you to install qemu-img, without
pulling in the rest of QEMU. In fact the storage pool driver RPM
depends on qemu-img explicitly since we dropped support for the
Xen tools for image creation a while ago.
There is scope for something to replace the current libvirt probing
code, but spawning 'qemu-img info' is certainly not that something.
Libvirt (and apps above libvirt in general) would really benefit from
having a library that they can use for readonly querying of information
about disk images. Of course that library can't just spawn qemu-img
otherwise that defeats the point of using a library.
Unfortunately QEMU's block layer can't practically serve this role
because its GPLv2-only licensing is too restrictive for some apps
needs.
It would have to be something conceptually similar in complexity to
what libvirt's current probing code does, so that we can have good
confidence in its behaviour in the face of malicious input.
I'll reiterate the historical state of the problem because I
think it's
important:
Pre-blockdev:
- we internally assumed that if the image format of an backing image
was not present in the overlay it is 'raw'
- this influenced security labelling but not actually how qemu viewed
or probed the file. If it was qcow2 probed as qcow2 qemu opened it
as qcow2 possibly even including the backing file if selinux or
other mechanism didn't prevent it.
post-blockdev:
- the assumption of 'raw' would now be expressed into the qemu
configuration. This assumption turned into data corruption since we
no longer allowed qemu to probe the format and forced it as raw.
- fix was to always require the format to be recorded in the overlay
- this made users unhappy who neglected to record the format into the
overlay when creating it manually
So the key problem we have is that with -blockdev we are always explicitly
telling QEMU what the backing file is for every image.
Can we fix this to have the exact same behaviour as before by *not* telling
QEMU anything about the backing file when using -blockdev, if there is no
well defined backing format present. ie, use -blockdev, but let QEMU probe
just as it did in non-blockdev days.
Would there be any downsides to this that did not already exist in the
non-blockdev days ?
I don't think we can solve the regressions in behaviour of backing files
by doing probing of the backing files in libvirt, because that only works
for the case where libvirt can actually open the file. ie a local file on
disk. We don't have logic for opening backing files on RBD, GlusterFS,
iSCSI, HTTP, SSH, etc, and nor do we want todo that.
So to me it looks like the only viable option is to not specify the
backing file info to QEMU at all.
Now this adds an interresting dimension to this problem. If libvirt
forces the users to specify the image format, and the users don't know
it they will probe. So we are basically making this a problem of
somebody else. [2] As you can see in that patch, it uses 'qemu-img'
anyways and also additionally actually allows the chain to continue
deeper! [3]
Yeah, this is a really bad situation given the difficulty in safely
using qemu-img, without also breaking valid usage.
We don't want to push this off to apps
This boils down to whether we want to accept some possibility of
image
corruption in trade for avoiding regression of behaviour in the secure
cases as well as management apps and users not having to re-invent when
probing an image is actually safe.
I feel like the risk of image corruption is pretty minor. Our probing
handles all normal cases the same way as QEMU and newly introduced
image formats are rare.
Finally I think we should either decide to fix it in this release, or
stick with the error message forever. Fixing it later will not make
much sense as many users already fixed their scripts and we'd just add
back the trade-off of possible image corruption.
Peter
[1] If e.g. the security subsystem of the host didn't forbid the use of
the backing file such a qcow2 qemu would happily open it.
[2] https://www.redhat.com/archives/libguestfs/2020-February/msg00013.html
[3] As implemented in [2] the backing image is not checked whether it
has a backing file or not but the format is probed, which way result
into accessing the backing chain of the probed image.
Prior to this detection, it would be prevented by sVirt or alternatively
also by libvit itself in -blockdev mode when this patch would be
accepted.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:10 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Mon, Feb 24, 2020 at 14:24:15 +0000, Daniel Berrange wrote:
On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:
> On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
[...]
> I'll reiterate the historical state of the problem because I
think it's
> important:
>
> Pre-blockdev:
> - we internally assumed that if the image format of an backing image
> was not present in the overlay it is 'raw'
> - this influenced security labelling but not actually how qemu viewed
> or probed the file. If it was qcow2 probed as qcow2 qemu opened it
> as qcow2 possibly even including the backing file if selinux or
> other mechanism didn't prevent it.
>
> post-blockdev:
> - the assumption of 'raw' would now be expressed into the qemu
> configuration. This assumption turned into data corruption since we
> no longer allowed qemu to probe the format and forced it as raw.
> - fix was to always require the format to be recorded in the overlay
> - this made users unhappy who neglected to record the format into the
> overlay when creating it manually
So the key problem we have is that with -blockdev we are always explicitly
telling QEMU what the backing file is for every image.
Can we fix this to have the exact same behaviour as before by *not* telling
QEMU anything about the backing file when using -blockdev, if there is no
well defined backing format present. ie, use -blockdev, but let QEMU probe
just as it did in non-blockdev days.
Would there be any downsides to this that did not already exist in the
non-blockdev days ?
We can, but the price is that:
1) we won't allow blockjobs and anything blockdev-related because node
name would be out of our control. This was possible in pre-blockdev era.
2) we will lose control of actually telling qemu to NOT open the backing
file in that case. Distros using only unix permission still have
arbitrary file access under permissions of the qemu process.
3) weird special-case code, because we need to keep some metadata about
the image to do security labelling
I don't think we can solve the regressions in behaviour of
backing files
by doing probing of the backing files in libvirt, because that only works
for the case where libvirt can actually open the file. ie a local file on
disk. We don't have logic for opening backing files on RBD, GlusterFS,
iSCSI, HTTP, SSH, etc, and nor do we want todo that.
Now we are back in the teritory where we actually do match what would
happen with previously. We don't specify these on the command line with
ehaviour matching what's described above, with the caveats as above.
I kept this behaviour because we couldn't do better. This is in place
even now if the last introspectable image has valid format specified.
We can reconsider how to approach this but ideally separately.
So to me it looks like the only viable option is to not specify the
backing file info to QEMU at all.
> Now this adds an interresting dimension to this problem. If libvirt
> forces the users to specify the image format, and the users don't know
> it they will probe. So we are basically making this a problem of
> somebody else. [2] As you can see in that patch, it uses 'qemu-img'
> anyways and also additionally actually allows the chain to continue
> deeper! [3]
Yeah, this is a really bad situation given the difficulty in safely
using qemu-img, without also breaking valid usage.
We don't want to push this off to apps
> This boils down to whether we want to accept some possibility of image
> corruption in trade for avoiding regression of behaviour in the secure
> cases as well as management apps and users not having to re-invent when
> probing an image is actually safe.
I feel like the risk of image corruption is pretty minor. Our probing
handles all normal cases the same way as QEMU and newly introduced
image formats are rare.
Well, in this case I'm actually for re-considering the original patch
discussed here. It uses image-format-probing code from libvirt, to allow
the most common cases which were forbidden in a safe way. This means
that as long as we can probe the image and the probed image does not
have a backing file we allow the startup.
It restores previous behaviour for valid cases including blockjobs,
correctly revokes invalid cases (existing chain after image wihtout
format, images impossible to introspect), is limited to the backing
store walking code so can be contained and the price is doing the image
format detection using libvirt's code.
6:50 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Mon, Feb 24, 2020 at 06:10:46PM +0100, Peter Krempa wrote:
On Mon, Feb 24, 2020 at 14:24:15 +0000, Daniel Berrange wrote:
> On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:
> > On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
[...]
> > I'll reiterate the historical state of the problem because I think
it's
> > important:
> >
> > Pre-blockdev:
> > - we internally assumed that if the image format of an backing image
> > was not present in the overlay it is 'raw'
> > - this influenced security labelling but not actually how qemu viewed
> > or probed the file. If it was qcow2 probed as qcow2 qemu opened it
> > as qcow2 possibly even including the backing file if selinux or
> > other mechanism didn't prevent it.
> >
> > post-blockdev:
> > - the assumption of 'raw' would now be expressed into the qemu
> > configuration. This assumption turned into data corruption since we
> > no longer allowed qemu to probe the format and forced it as raw.
> > - fix was to always require the format to be recorded in the overlay
> > - this made users unhappy who neglected to record the format into the
> > overlay when creating it manually
>
> So the key problem we have is that with -blockdev we are always explicitly
> telling QEMU what the backing file is for every image.
>
> Can we fix this to have the exact same behaviour as before by *not* telling
> QEMU anything about the backing file when using -blockdev, if there is no
> well defined backing format present. ie, use -blockdev, but let QEMU probe
> just as it did in non-blockdev days.
>
> Would there be any downsides to this that did not already exist in the
> non-blockdev days ?
We can, but the price is that:
1) we won't allow blockjobs and anything blockdev-related because node
name would be out of our control. This was possible in pre-blockdev era.
Ok, that's not viable then. We can't switch one regression for a different
regression.
2) we will lose control of actually telling qemu to NOT open the
backing
file in that case. Distros using only unix permission still have
arbitrary file access under permissions of the qemu process.
True, but that is at least historically expected behaviour, which can
be fixed by setting <backingfile/> in the XML file IIUC.
3) weird special-case code, because we need to keep some metadata
about
the image to do security labelling
> I don't think we can solve the regressions in behaviour of backing files
> by doing probing of the backing files in libvirt, because that only works
> for the case where libvirt can actually open the file. ie a local file on
> disk. We don't have logic for opening backing files on RBD, GlusterFS,
> iSCSI, HTTP, SSH, etc, and nor do we want todo that.
Now we are back in the teritory where we actually do match what would
happen with previously. We don't specify these on the command line with
ehaviour matching what's described above, with the caveats as above.
I kept this behaviour because we couldn't do better. This is in place
even now if the last introspectable image has valid format specified.
We can reconsider how to approach this but ideally separately.
I'm a little lost as to exactly which scenarios are broken, and which
we're fixing.
1. file:top.qcow2 -> file:base.raw
2. file:top.qcow2 -> file:base.qcow2
3. file:top.qcow2 -> file:middle.qcow2 -> file:base.raw
4. file:top.qcow2 -> file:middle.qcow2 -> file:base.qcow2
IIUC, (1) is working before and now, (2) is working before but
broken now, and (3) and (4) were broken before and now.
So (2) is the only one we /must/ to fix
Am I right that by doing probing in libvirt as per this patch,
as well as fixing (2) though, we'll accidentally fix (3) and (4)
even though they were always broken before ?
This all talks about qcow2 images on the file: protocol driver.
What is the situation for, say, the iscsi: protocol driver
1. iscsi:top.qcow2 -> iscsi:base.raw
2. iscsi:top.qcow2 -> iscsi:base.qcow2
3. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.raw
4. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.qcow2
What was the behaviour of this in the pre-blockdev days and
vs current git master ? Is it the same with (1) is working
before and now, (2) is working before but broken now, and
(3) and (4) were broken before and now.
I'm assuming the libvirt probing cannot fix any case other
than file: protocol, or host-device: protocol, since we're
unable to open any other type of storage.
> > This boils down to whether we want to accept some
possibility of image
> > corruption in trade for avoiding regression of behaviour in the secure
> > cases as well as management apps and users not having to re-invent when
> > probing an image is actually safe.
>
> I feel like the risk of image corruption is pretty minor. Our probing
> handles all normal cases the same way as QEMU and newly introduced
> image formats are rare.
Well, in this case I'm actually for re-considering the original patch
discussed here. It uses image-format-probing code from libvirt, to allow
the most common cases which were forbidden in a safe way. This means
that as long as we can probe the image and the probed image does not
have a backing file we allow the startup.
It restores previous behaviour for valid cases including blockjobs,
correctly revokes invalid cases (existing chain after image wihtout
format, images impossible to introspect), is limited to the backing
store walking code so can be contained and the price is doing the image
format detection using libvirt's code.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
7:21 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On Tue, Feb 25, 2020 at 12:50:21 +0000, Daniel Berrange wrote:
On Mon, Feb 24, 2020 at 06:10:46PM +0100, Peter Krempa wrote:
> On Mon, Feb 24, 2020 at 14:24:15 +0000, Daniel Berrange wrote:
> > On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:
[...]
> > Would there be any downsides to this that did not already
exist in the
> > non-blockdev days ?
>
> We can, but the price is that:
> 1) we won't allow blockjobs and anything blockdev-related because node
> name would be out of our control. This was possible in pre-blockdev era.
Ok, that's not viable then. We can't switch one regression for a different
regression.
> 2) we will lose control of actually telling qemu to NOT open the backing
> file in that case. Distros using only unix permission still have
> arbitrary file access under permissions of the qemu process.
True, but that is at least historically expected behaviour, which can
be fixed by setting <backingfile/> in the XML file IIUC.
Yes, but if you specify any <backingStore> including the terminator you
basically configure the image chain yourselves. The part which is
configured explicitly will not undergo any form of detection, not even
looking for the backing file.
> 3) weird special-case code, because we need to keep some
metadata about
> the image to do security labelling
>
> > I don't think we can solve the regressions in behaviour of backing files
> > by doing probing of the backing files in libvirt, because that only works
> > for the case where libvirt can actually open the file. ie a local file on
> > disk. We don't have logic for opening backing files on RBD, GlusterFS,
> > iSCSI, HTTP, SSH, etc, and nor do we want todo that.
>
> Now we are back in the teritory where we actually do match what would
> happen with previously. We don't specify these on the command line with
> ehaviour matching what's described above, with the caveats as above.
>
> I kept this behaviour because we couldn't do better. This is in place
> even now if the last introspectable image has valid format specified.
>
> We can reconsider how to approach this but ideally separately.
I'm a little lost as to exactly which scenarios are broken, and which
we're fixing.
1. file:top.qcow2 -> file:base.raw
2. file:top.qcow2 -> file:base.qcow2
3. file:top.qcow2 -> file:middle.qcow2 -> file:base.raw
4. file:top.qcow2 -> file:middle.qcow2 -> file:base.qcow2
I assume you meant that none of the 'qcow2' files have format of the
backing file recorded in the metadata.
IIUC, (1) is working before and now, (2) is working before but
broken now, and (3) and (4) were broken before and now.
1) was working before, but is now forbidden
2) was working before, and is now forbidden
3,4) were possibly working (without sVirt), or would get permission
denied reported by qemu during startup, currently they are forbidden by
a libvirt error message
After this patch:
1) will be fixed by this patch
2) will be fixed by this patch
3, 4) will report a libvirt error rather than relying on sVirt or others
So (2) is the only one we /must/ to fix
Am I right that by doing probing in libvirt as per this patch,
as well as fixing (2) though, we'll accidentally fix (3) and (4)
even though they were always broken before ?
This all talks about qcow2 images on the file: protocol driver.
What is the situation for, say, the iscsi: protocol driver
1. iscsi:top.qcow2 -> iscsi:base.raw
2. iscsi:top.qcow2 -> iscsi:base.qcow2
3. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.raw
4. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.qcow2
What was the behaviour of this in the pre-blockdev days and
vs current git master ? Is it the same with (1) is working
before and now, (2) is working before but broken now, and
(3) and (4) were broken before and now.
Pre-blockdev and post-blockdev every scenario of the above is working.
We can't even inspect the top file for backing store so everything is
just ignored.
Now what partially changes is when we have something introspectable in
the way:
1,2) file:top.qcow2 -> iscsi:base.whatever
The current code would report an error if the format was not recorded
in the overlay (top.qcow2) and we can't introspect the backing file
we'll reject it.
This can be relaxed though.
3,4) will work if top.qcow2 has the format but any subsequent don't since
we don't introspect it
I'm assuming the libvirt probing cannot fix any case other
than file: protocol, or host-device: protocol, since we're
unable to open any other type of storage.
It fixes also gluster since the backends for that are already
implemented.
I'll post another rebased version with some updated docs and one fix. We
can continue there perhaps.
10:44 a.m.
New subject: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
On 2/19/20 10:21 AM, Eric Blake wrote:
It took me a few minutes of thinking about this.
Scenario 1:
base.raw <- wrap.qcow2
where wrap.qcow2 specifies backing of base.raw but not the format. If
we probe, we can have a couple of outcomes:
1a: base.raw probes as raw (the probed image has no backing or data
file), using it as raw is safe (it matches what wrap.qcow2 should have
specified but didn't, and we aren't changing the data the guest would
read nor are we opening unexpected files)
1b: base.raw probes as qcow2 (because of whatever the guest wrote
there), using it as qcow2 is wrong - the guest will see corrupted data.
What's more, if the probe sees it as qcow2 with backing file, and we
open the backing file, it also has security implications.
Scenario 2:
base.qcow2 <- wrap.qcow2
where wrap.qcow2 specifies backing of base.qcow2 but not the format. If
we probe, we will always have just one outcome:
2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if
base.qcow2 has a further backing image, the backing chain is now
dependent on a probe.
Since 1b and 2a have the same probe result, but massively different data
corruption and/or security concerns, it is NOT sufficient to claim that
a probe was safe merely because "the probed image does not have any
backing or data file". It is ONLY safe if the probe turns up raw. Any
other probed format is inherently unsafe.
Having said that, I can offer this heuristic:
If the file name itself implies a particular format was intended (such
as naming a file *.raw or *.qcow2), and the probe matches what the
suffix would imply, we can probably guess that the user did really mean
that format (we could even allow *.qcow to resolve to a probe of qcow2,
even though qcow and qcow2 are different probe results). File names
that do not imply a format (such as 'bare' or 'base.img' or ...) cannot
benefit from heuristics.
But we just got rid of suffix heuristics in patch 1 of this series. So
using file suffix hueristics to accept a filename of 'base.qcow2' that
probed as qcow2 as probably being safe, in spite of the missing format,
is risky business. While a heuristic might let more pre-existing cases
of missing formats boot without data corruption, we have to consider
whether the extra magic is helpful or will get in the way for the cases
where a management app misnamed their file (calling a file 'base.qcow2'
when it is really raw) and such misnaming causes our heuristics to allow
data corruption through to the guest.
Extra magic that can't go wrong is one thing, but extra magic that can
risk data corruption in corner cases where a management app was not
careful with their file names is probably not worth the maintenance
effort. So I agree with patch 1 removing suffix probing, and think it
is not worth trying the file name heuristics just to help management
apps that forgot their backing format.
I disagree with the logic here. What we really need is:
If the backing format was not specified, we probe to see what is there.
If the result of that probe is raw, it is safe to treat the image as
raw. If the result is anything else, we must report an error stating
that what we probed could not be trusted unless the user adds an
explicit backing format (either confirming that our probe was correct,
or with the correct format overriding what we mis-probed).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:13 a.m.
New subject: [PATCH 9/9] WIP: Add tool for probing images
Note that this is not finished yet, but allows to test the image
detection patches:
Prepare few images:
qemu-img create -f qcow2 /tmp/base.qcow2 10M
qemu-img create -f qcow2 -b /tmp/base.qcow2 /tmp/overlay1-noformat.qcow2
qemu-img create -f qcow2 -F qcow2 -b /tmp/base.qcow2 /tmp/overlay1-format.qcow2
qemu-img create -f qcow2 -F qcow2 -b /tmp/overlay1-format.qcow2
/tmp/overlay2-format.qcow2
qemu-img create -f qcow2 -b /tmp/overlay1-noformat.qcow2
/tmp/overlay2-noformat.qcow2
qemu-img creage -f qcow2 -b nbd://example/asdf /tmp/nbd-noformat.qcow2 10M
(Note that the last one prints error, but that's expected)
Probe images:
$ ./tests/qemublockprobe -f qcow2 -p /tmp/overlay1-noformat.qcow2
type: file (1)
path: /tmp/overlay1-noformat.qcow2
format: qcow2 (14)
protocol: none' (0)
backing store raw: /tmp/base.qcow2
type: file (1)
path: /tmp/base.qcow2
format: qcow2 (14)
protocol: none' (0)
type: none (0)
path: (null)
format: none (0)
protocol: none' (0)
$ ./tests/qemublockprobe -f qcow2 -p /tmp/overlay2-format.qcow2
type: file (1)
path: /tmp/overlay2-format.qcow2
format: qcow2 (14)
protocol: none' (0)
backing store raw: /tmp/overlay1-format.qcow2
type: file (1)
path: /tmp/overlay1-format.qcow2
format: qcow2 (14)
protocol: none' (0)
backing store raw: /tmp/base.qcow2
type: file (1)
path: /tmp/base.qcow2
format: qcow2 (14)
protocol: none' (0)
type: none (0)
path: (null)
format: none (0)
protocol: none' (0)
$ ./tests/qemublockprobe -f qcow2 -p /tmp/overlay2-noformat.qcow2
/home/pipo/build/libvirt/gcc/tests/.libs/lt-qemublockprobe: libvirt error: Requested
operation is not valid: format of backing image '/tmp/overlay1-noformat.qcow2' of
image '/tmp/overlay2-noformat.qcow2' was not specified in the image metadata (See
https://libvirt.org/kbase/backing_chains.html for troubleshooting)
$ ./tests/qemublockprobe -f qcow2 -p /tmp/nbd-noformat.qcow2
/home/pipo/build/libvirt/gcc/tests/.libs/lt-qemublockprobe: libvirt error: Requested
operation is not valid: format of backing image 'nbd://example/asdf' of image
'/tmp/nbd-noformat.qcow2' was not specified in the image metadata (See
https://libvirt.org/kbase/backing_chains.html for troubleshooting)
---
tests/Makefile.am | 13 ++++-
tests/qemublockprobe.c | 130 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 142 insertions(+), 1 deletion(-)
create mode 100644 tests/qemublockprobe.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ed5255b62d..a47c7eda22 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -291,7 +291,9 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \
qemufirmwaretest \
qemuvhostusertest \
$(NULL)
-test_helpers += qemucapsprobe
+test_helpers += qemucapsprobe \
+ qemublockprobe \
+ $(NULL)
test_libraries += libqemumonitortestutils.la \
libqemutestdriver.la \
libqemuxml2argvmock.la \
@@ -652,6 +654,14 @@ qemublocktest_LDADD = \
$(qemu_LDADDS) \
$(NULL)
+qemublockprobe_SOURCES = \
+ qemublockprobe.c \
+ $(NULL)
+qemublockprobe_LDADD = \
+ ../src/libvirt.la \
+ $(qemu_LDADDS) \
+ $(NULL)
+
qemudomaincheckpointxml2xmltest_SOURCES = \
qemudomaincheckpointxml2xmltest.c testutilsqemu.c testutilsqemu.h \
testutils.c testutils.h
@@ -707,6 +717,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c \
qemucaps2xmltest.c qemucommandutiltest.c \
qemumemlocktest.c qemucpumock.c testutilshostcpus.h \
qemublocktest.c \
+ qemublockprobe.c \
qemumigparamstest.c \
qemusecuritytest.c qemusecuritytest.h \
qemusecuritymock.c \
diff --git a/tests/qemublockprobe.c b/tests/qemublockprobe.c
new file mode 100644
index 0000000000..0dbc31028c
--- /dev/null
+++ b/tests/qemublockprobe.c
@@ -0,0 +1,130 @@
+/*
+ * qemublockprobe.c: image backing chain prober
+ *
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdbool.h>
+
+#include "util/virfile.h"
+#include "util/virlog.h"
+#include "util/virstoragefile.h"
+
+#include "virgettext.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+
+static void
+print_source(virStorageSourcePtr src)
+{
+ size_t i;
+
+ g_print("type: %s (%d)\n", virStorageTypeToString(src->type),
src->type);
+ g_print("path: %s\n", src->path);
+ g_print("format: %s (%d)\n",
virStorageFileFormatTypeToString(src->format), src->format);
+ g_print("protocol: %s' (%d)\n",
virStorageNetProtocolTypeToString(src->protocol), src->protocol);
+ for (i = 0; i < src->nhosts; i++) {
+ virStorageNetHostDefPtr h = src->hosts + i;
+
+ g_print("host %zu: name: '%s', port: '%u', transport:
'%s'(%d), socket: '%s'\n",
+ i, h->name, h->port,
virStorageNetHostTransportTypeToString(h->transport),
+ h->transport, h->socket);
+ }
+ if (src->sliceStorage)
+ g_print("slice type: storage, offset: %llu, size: %llu\n",
+ src->sliceStorage->offset, src->sliceStorage->size);
+ if (src->backingStoreRaw)
+ g_print("backing store raw: %s\n", src->backingStoreRaw);
+ if (src->externalDataStoreRaw)
+ g_print("external store raw: %s\n", src->externalDataStoreRaw);
+ if (src->relPath)
+ g_print("relative path: %s\n", src->relPath);
+
+ g_print("\n");
+}
+
+
+int main(int argc, char **argv)
+{
+ g_autofree char *path = NULL;
+ g_autofree char *format = NULL;
+ g_autoptr(GError) error = NULL;
+ bool verbose = false;
+ g_autoptr(virStorageSource) src = NULL;
+ GOptionContext *ctx;
+ virStorageSourcePtr n;
+ int ret = 1;
+
+ GOptionEntry entries[] = {
+ { "path", 'p', 0, G_OPTION_ARG_STRING, &path, "path to
image", "DIR" },
+ { "format", 'f', 0, G_OPTION_ARG_STRING, &format,
"format of image", "DIR" },
+ { "verbose", 'v', 0, G_OPTION_ARG_NONE, &verbose,
"Verbose output", NULL },
+ { 0 }
+ };
+
+
+ ctx = g_option_context_new("- inspect an image");
+ g_option_context_add_main_entries(ctx, entries, PACKAGE);
+ if (!g_option_context_parse(ctx, &argc, &argv, &error)) {
+ g_printerr("%s: option parsing failed: %s\n",
+ argv[0], error->message);
+ return 1;
+ }
+
+ if (!path) {
+ g_printerr("%s: missing path\n", argv[0]);
+ return 1;
+ }
+
+ if (virErrorInitialize() < 0) {
+ g_printerr("%s: failed to initialize error handling\n", argv[0]);
+ return 1;
+ }
+
+ virLogSetFromEnv();
+ virFileActivateDirOverrideForProg(argv[0]);
+
+ if (!(src = virStorageSourceNew()))
+ goto cleanup;
+
+ src->path = g_steal_pointer(&path);
+ src->type = VIR_STORAGE_TYPE_FILE;
+
+ if (format &&
+ (src->format = virStorageFileFormatTypeFromString(format)) < 0) {
+ g_printerr("%s: unknown format '%s'\n", argv[0], format);
+ goto cleanup;
+ }
+
+ if (virStorageFileGetMetadata(src, -1, -1, true) < 0)
+ goto cleanup;
+
+ for (n = src; n; n = n->backingStore)
+ print_source(n);
+
+ ret = 0;
+
+ cleanup:
+ if (virGetLastErrorCode() != VIR_ERR_OK)
+ g_printerr("%s: libvirt error: %s\n", argv[0],
virGetLastErrorMessage());
+
+ return ret;
+}
--
2.24.1
10:31 a.m.
New subject: [PATCH 9/9] WIP: Add tool for probing images
On 2/17/20 11:13 AM, Peter Krempa wrote:
Note that this is not finished yet, but allows to test the image
detection patches:
"allows to ${verb}" is not idiomatic; you want "allows ${verb}ing" or
"allows $subject to ${verb}". Here, I would go with "allows testing of
the image detection patches".
Prepare few images:
Prepare a few images:
qemu-img create -f qcow2 /tmp/base.qcow2 10M
qemu-img create -f qcow2 -b /tmp/base.qcow2 /tmp/overlay1-noformat.qcow2
qemu-img create -f qcow2 -F qcow2 -b /tmp/base.qcow2 /tmp/overlay1-format.qcow2
qemu-img create -f qcow2 -F qcow2 -b /tmp/overlay1-format.qcow2
/tmp/overlay2-format.qcow2
qemu-img create -f qcow2 -b /tmp/overlay1-noformat.qcow2
/tmp/overlay2-noformat.qcow2
qemu-img creage -f qcow2 -b nbd://example/asdf /tmp/nbd-noformat.qcow2 10M
/tmp/overlay1-noformat.qcow2 is inherently unsafe. The probe of
/tmp/base.qcow2 returns qcow2, but we cannot trust whether that was
because /tmp/base.qcow2 was actually qcow2 or if it was because
/tmp/base.qcow2 was raw where the guest wrote a qcow2 header; in the
former case our guess is correct, but in the latter case, even though we
avoid a security issue of chasing further files under guest control, we
do NOT avoid the issue of corrupting guest data (serving the qcow2
payload rather than the qcow2 metadata that the guest wrote in a raw
file is guest-visible data corruption).
(Note that the last one prints error, but that's expected)
Probe images:
$ ./tests/qemublockprobe -f qcow2 -p /tmp/overlay1-noformat.qcow2
type: file (1)
path: /tmp/overlay1-noformat.qcow2
format: qcow2 (14)
protocol: none' (0)
Why the mismatched '?
backing store raw: /tmp/base.qcow2
type: file (1)
path: /tmp/base.qcow2
format: qcow2 (14)
protocol: none' (0)
type: none (0)
path: (null)
format: none (0)
protocol: none' (0)
The tool needs to report that this image as potentially corrupt (our
probe of qcow2 may be correct, or it may be a mistake for what was
really raw, and without an explicit backing format, we are unwilling to
hand the image to qemu for fear of data corruption visible to the guest,
even if we have avoided a security hole of chasing files under guest
control).
$ ./tests/qemublockprobe -f qcow2 -p /tmp/overlay2-format.qcow2
type: file (1)
path: /tmp/overlay2-format.qcow2
format: qcow2 (14)
protocol: none' (0)
backing store raw: /tmp/overlay1-format.qcow2
type: file (1)
path: /tmp/overlay1-format.qcow2
format: qcow2 (14)
protocol: none' (0)
backing store raw: /tmp/base.qcow2
type: file (1)
path: /tmp/base.qcow2
format: qcow2 (14)
protocol: none' (0)
type: none (0)
path: (null)
format: none (0)
protocol: none' (0)
This image is safe.
$ ./tests/qemublockprobe -f qcow2 -p /tmp/overlay2-noformat.qcow2
/home/pipo/build/libvirt/gcc/tests/.libs/lt-qemublockprobe: libvirt error: Requested
operation is not valid: format of backing image '/tmp/overlay1-noformat.qcow2' of
image '/tmp/overlay2-noformat.qcow2' was not specified in the image metadata (See
https://libvirt.org/kbase/backing_chains.html for troubleshooting)
This image is correctly identified as unsafe.
$ ./tests/qemublockprobe -f qcow2 -p /tmp/nbd-noformat.qcow2
/home/pipo/build/libvirt/gcc/tests/.libs/lt-qemublockprobe: libvirt error: Requested
operation is not valid: format of backing image 'nbd://example/asdf' of image
'/tmp/nbd-noformat.qcow2' was not specified in the image metadata (See
https://libvirt.org/kbase/backing_chains.html for troubleshooting)
This image is correctly identified as potentially unsafe because we were
unable to probe nbd://example/asdf (had the probe been successful, AND
returned a result of raw, then this image would be safe; had the probe
been successful but returned anything other than raw, it is no different
than the existing failure of the probe being unsuccessful)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:45 a.m.
New subject: [PATCH 9/9] WIP: Add tool for probing images
On Wed, Feb 19, 2020 at 10:31:18 -0600, Eric Blake wrote:
On 2/17/20 11:13 AM, Peter Krempa wrote:
> Note that this is not finished yet, but allows to test the image
> detection patches:
"allows to ${verb}" is not idiomatic; you want "allows ${verb}ing"
or
"allows $subject to ${verb}". Here, I would go with "allows testing of
the
image detection patches".
>
> Prepare few images:
Prepare a few images:
> qemu-img create -f qcow2 /tmp/base.qcow2 10M
> qemu-img create -f qcow2 -b /tmp/base.qcow2 /tmp/overlay1-noformat.qcow2
> qemu-img create -f qcow2 -F qcow2 -b /tmp/base.qcow2 /tmp/overlay1-format.qcow2
> qemu-img create -f qcow2 -F qcow2 -b /tmp/overlay1-format.qcow2
/tmp/overlay2-format.qcow2
> qemu-img create -f qcow2 -b /tmp/overlay1-noformat.qcow2
/tmp/overlay2-noformat.qcow2
> qemu-img creage -f qcow2 -b nbd://example/asdf /tmp/nbd-noformat.qcow2 10M
>
/tmp/overlay1-noformat.qcow2 is inherently unsafe. The probe of
/tmp/base.qcow2 returns qcow2, but we cannot trust whether that was because
/tmp/base.qcow2 was actually qcow2 or if it was because /tmp/base.qcow2 was
raw where the guest wrote a qcow2 header; in the former case our guess is
correct, but in the latter case, even though we avoid a security issue of
chasing further files under guest control, we do NOT avoid the issue of
corrupting guest data (serving the qcow2 payload rather than the qcow2
metadata that the guest wrote in a raw file is guest-visible data
corruption).
> (Note that the last one prints error, but that's expected)
>
> Probe images:
>
> $ ./tests/qemublockprobe -f qcow2 -p /tmp/overlay1-noformat.qcow2
> type: file (1)
> path: /tmp/overlay1-noformat.qcow2
> format: qcow2 (14)
> protocol: none' (0)
Why the mismatched '?
> backing store raw: /tmp/base.qcow2
>
> type: file (1)
> path: /tmp/base.qcow2
> format: qcow2 (14)
> protocol: none' (0)
>
> type: none (0)
> path: (null)
> format: none (0)
> protocol: none' (0)
>
The tool needs to report that this image as potentially corrupt (our probe
of qcow2 may be correct, or it may be a mistake for what was really raw, and
without an explicit backing format, we are unwilling to hand the image to
qemu for fear of data corruption visible to the guest, even if we have
avoided a security hole of chasing files under guest control).
As noted previously, we've used these semantics forever. Prior to
introduction of blockdev, we probed the format, but assumed that the
image is 'raw' in such case. But we've never reported an error or done
anything. We started the VM and let qemu re-probe.
This meant that both 'raw' and 'qcow2' images without a backing file
were working without a hitch even with sVirt. Users not having the
benefit of sVirt were also possibly still seeing possible unwanted
access.
I'm specifically interrested in keeping both 'raw' and single layer
'qcow2' work as they did before because in my opinion it's not worse
than it was before when running qemu where libvirt wouldn't use blockdev
and _strictly_ better in case when we do use -blockdev, while not making
many users unhappy.
And there were already a lot of unhappy users.
1732
days inactive
1740
days old
31 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Peter Krempa