On Tue, Feb 25, 2020 at 02:25:44PM +0100, 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
> are able to probe the image, the probing result was successful and the
> probed image does not have any backing or data file.
Ok, so we're limiting probing to 1 level of recursion, so we're
not newly enabling 3 level chains
>
> 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.
>
> Users thus don't have to re-invent when image format detection is safe
> 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.
>
> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
> 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 d75d2a689a..b133cf17f1 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -4986,6 +4986,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
> virHashTablePtr cycle,
> unsigned int depth)
> {
> + virStorageFileFormat orig_format = src->format;
> size_t headerLen;
> int rv;
> g_autofree char *buf = NULL;
> @@ -4995,10 +4996,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)
> @@ -5007,6 +5015,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
> if (virStorageFileGetMetadataInternal(src, buf, headerLen) < 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;
> + }
> + }
Ok, this is where we limit recursion to 1 level to avoid enabling
deep chains.
> +
> if (src->backingStoreRaw) {
> if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
> return -1;
> @@ -5015,33 +5035,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
> if (rv == 1)
> return 0;
>
> - 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;
> - }
And this is just about improved error reporting
> -
> - 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;
Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
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 :|
