Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Eric Blake]

[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

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Peter Krempa]

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.

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Daniel P. Berrangé]

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 :|

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Peter Krempa]

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.

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Daniel P. Berrangé]

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 :|

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Peter Krempa]

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.