Re: [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk
On Mon, Mar 09, 2020 at 10:32:52AM -0500, Eric Blake wrote:
On 3/9/20 10:21 AM, Kevin Wolf wrote:
> Am 06.03.2020 um 23:51 hat Eric Blake geschrieben:
> > For qcow2 and qed, we want to encourage the use of -F always, as these
> > formats can suffer from data corruption or security holes if backing
> > format is probed. But for other formats, the backing format cannot be
> > recorded. Making the user decide on a per-format basis whether to
> > supply a backing format string is awkward, better is to just blindly
> > accept a backing format argument even if it is ignored by the
> > contraints of the format at hand.
> >
> > Signed-off-by: Eric Blake <eblake(a)redhat.com>
>
> I'm not sure if I agree with this reasoning. Accepting and silently
> ignoring -F could give users a false sense of security. If I specify a
> -F raw and QEMU later probes qcow2, that would be very surprising.
Do we know what formats qcow, sheepdog, and vmdk expect to probe? I'm
wondering if we can compromise by checking that the requested backing image
has the specified format, and error if it is not, rather than completely
ignoring it - but at the same time, the image formats have no where to
record a backing format.
Consider the user creates an image with "-F raw". We can validate the backing
image is raw, and so our check succeeds. Later the malicious <something> can
write a qcow header into this raw file and QEMU will thereafter probe the
image as qcow, not raw.
IOW, in the case of "-F raw", even if we immediately check the format,
we're
still not offering the protection promised by the "-F" flag, because that
promise refers to the runtime behaviour of the QEMU emulator, not the
immediate qemu-img cmd.
We could support "-F ..." and validate any non-raw formats, while raising a
runtime error in the case of "-F raw", as only the "raw" backing
format has
the probing security risk.
Users who need to use qcow, with a backing file, without a format can
just not pass "-F" and in doing so will be insecure.
We could take this opportunity to deprecate 'qcow' perhaps, declare it
a read-only format, restricted to qemu-img/qemu-io for purpose of data
liberation ?
For sheepdog, if it is something we genuinely still care about, then
adding a backing file format record seems neccessary, unless we either
forbid use of raw backing files, or forbid use of non-raw backing files,
either way would be safe.
I'm guessing that qcow works with either raw or qcow as backing
format (and
anything else is odd - a qcow2 backing to a qcow is unusual, and would be
better to reject). I'm not sure if sheepdog can be backed by anything but
another sheepdog, similarly, I'm not sure if a vmdk can be backed by
anything but another vmdk. If so, it should be simple enough to do a v4 of
this patch which requires -F to be a known-acceptable probe type for these
images.
Still, the point of this patch is that I want to add -F into all the
iotests, and without something along the lines of this patch, all of those
iotests are broken for these image formats. Patch 2 is a lot harder to
write if we have to make our use of -F conditional on the image format in
question.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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 :|