Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/9/20 10:31 AM, Kashyap Chamarthy wrote: On Fri, Mar 06, 2020 at 04:51:21PM -0600, Eric Blake wrote: Creating an image that requires format probing of the backing image is inherently unsafe (we've had several CVEs over the years based on +qemu-img backing file without format (since 5.0.0) +'' + +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img +convert``, or ``qemu-img amend`` to create or modify an image that +depends on a backing file now recommends that an explicit backing +format be provided. This is for safety: if qemu probes a different +format than what you thought, the data presented to the guest will be +corrupt; similarly, presenting a raw image to a guest allows a +potential security exploit if a future probe sees a non-raw image +based on guest writes. To avoid the warning message, or even future +refusal to create an unsafe image, you must pass ``-o backing_fmt=`` +(or the shorthand ``-F`` during create) to specify the intended +backing format. You may use ``qemu-img rebase -u`` to retroactively +add a backing format to an existing image. However, be aware that +there are already potential security risks to blindly using ``qemu-img +info`` to probe the format of an untrusted backing image, when +deciding what format to add into an existing image. Nit: s/qemu/QEMU/g/ Ultra Nit: should this paragraph be broken down into two? Experience tells people usually feel deterred read "substantial paragraphs" :-) Shoot, I missed incorporating this comment during my v4 posting. It's now changed in my local tree, but I'll hold off on a v5 unless other review warrants it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Tue, Mar 10, 2020 at 07:15:29AM -0500, Eric Blake wrote: > On 3/10/20 4:47 AM, Kashyap Chamarthy wrote: [...] > > > > > > Ah, didn't realize the inconsistency of 'convert' lacking the '-F' > > shorthand ... which reminds me, there are at least _three_ ways that I > > know of, to specify backing file format with 'create': > > > > $ qemu-img create -f qcow2 -o > > 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2 > > $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw > > overlay1.qcow2 > > $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2 > > > > I'm wondering about the consistency of having all the above three > > supported for other operations too. Now I at least know 'convert' lacks > > the "-F". > > The -o forms (backing_file= and backing_fmt=) always work. Various commands > then have additional shorthand: -b/-F for create, -B for convert. You're > right that we aren't very consistent, but I'm reluctant to change the > inconsistencies in this patch Oh, I wasn't implying to tackle the inconsistency as part of this patch, or series. Hence the 'digression' :-) Was just wondering out loud. > (at one point in the past, we tried to get rid > of the shorthand and force all users to go through -o, but that broke too > many clients that were depending on the undocumented shorthand, so we > documented the existing shorthand instead). Fair enough; let's not touch these things for now. -- /kashyap
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Tue, Mar 10, 2020 at 07:19:25AM -0500, Eric Blake wrote: > On 3/10/20 5:57 AM, Kashyap Chamarthy wrote: (Slightly long e-mail, as it contains a bunch of tests and their results; please bear with me.) [...] > > $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 > > This particular amend is not changing the backing file, and since I did not > add warnings on the opening of an existing unsafe image, it is silent. I see; okay, that's expected. > Instead, you need to test a case where amend provokes a path that would > change the backing file (perhaps as simple as '-o backing_file=./base.raw'), > while omitting a format for the new backing file string. I couldn't work out the black magic to change the backing file via 'qemu-img amend'. It is surely not this: $> ~/build/qemu/qemu-img amend -o 'backing_file=./bar.qcow2' -o another_base.qcow2 qemu-img: Expecting one image file name Let's try something else: give a *non-existent* "bar.qcow2" to '-o': $> ~/build/qemu/qemu-img amend -o 'backing_file=./bar.qcow2' another_base.qcow2 qemu-img: Could not open 'another_base.qcow2': Could not open backing file: Failed to get shared "write" lock Is another process using the image [./another_base.qcow2]? That's strange; there is no live QEMU process on this host (let alone one that is using another_base.qcow2): $> pgrep qemu-system-x86 $> echo $? 1 Probably it is just complaning about the non-existent bar.qcow2 file? Then I'd expect it to say as much. On IRC you pointed out iotest 082 to look for help. There I don't see a way to change the backing file. But only a combination of 'amend' + 'rebase': run_qemu_img amend -f $IMGFMT \ -o backing_fmt=$IMGFMT,backing_file="$TEST_IMG",,\? "$TEST_IMG" run_qemu_img rebase -u -b "" -f $IMGFMT "$TEST_IMG" (I know you can use 'rebase' alone to change the backing file format.) Note to self: we really need to document 'amend' much better, in which scenarios it is useful, and contrast it with 'rebase'. - - - Meanwhile, I've done a bunch of tests with 'amend'. Here are the results. Scenario: base.raw <-- overlay1.qcow2 - Without "-f raw", the warning is provoked when trying to amend the backing file (let's ignore for a moment that you can't seem to amend a raw file): $> ~/build/qemu/qemu-img amend -o compat=v3 base.raw WARNING: Image format was not specified for 'base.raw' and probing guessed raw. Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted. Specify the 'raw' format explicitly to remove the restrictions. qemu-img: Format driver 'raw' does not support option amendment With "-f raw", the warning is not triggerred (correctly so?): $> ~/build/qemu/qemu-img amend -o compat=v3 -f raw base.raw qemu-img: Format driver 'raw' does not support option amendment And these two tests (one with relative path; the other with absolute path) don't trigger the warning either (on IRC you said the following is _supposed_ to trigger a warning): $> ~/build/qemu/qemu-img amend \ -o 'backing_file=base.raw' -f qcow2 overlay1.qcow2 $> ~/build/qemu/qemu-img amend \ -o 'backing_file=./base.raw' -f qcow2 overlay1.qcow2 'qemu-img info' of the above disk image chain: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./base.raw backing file format: raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: ./base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB Scenario: another_base.qcow2 <-- overlay1_of_ab.qcow2 - With and w/o specifying the aAmend the backing file (none of these provoke the warning -- expected?): $> ~/build/qemu/qemu-img amend another_base.qcow2 $> ~/build/qemu/qemu-img amend -f qcow2 another_base.qcow2 Tests to amend the overlay file (none of these provoke the warning -- expected, per your previous reply): $> ~/build/qemu/qemu-img amend overlay1_of_ab.qcow2 $> ~/build/qemu/qemu-img amend -f qcow2 overlay1_of_ab.qcow2 'qemu-img info' of the above disk image chain: $> ~/build/qemu/qemu-img info --backing-chain overlay1_of_ab.qcow2 image: overlay1_of_ab.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./another_base.qcow2 Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: ./another_base.qcow2 file format: qcow2 virtual size:
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/10/20 5:57 AM, Kashyap Chamarthy wrote: On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: [...] +qemu-img backing file without format (since 5.0.0) +'' + +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img +convert``, or ``qemu-img amend`` to create or modify an image that +depends on a backing file now recommends that an explicit backing +format be provided. Also for the `qemu-img amend` case, I'm not clear if the following scenario ought to throw the warning: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 This particular amend is not changing the backing file, and since I did not add warnings on the opening of an existing unsafe image, it is silent. Instead, you need to test a case where amend provokes a path that would change the backing file (perhaps as simple as '-o backing_file=./base.raw'), while omitting a format for the new backing file string. $> echo $? 0 I'm just trying to work out the scenarios where the warnings ought to show up (for all the four cases: create, rebase, convert, amend). Look at patch 2/4 - that patch was written AFTER this patch in order to silence every warning that was introduced because of this patch, then rebased to occur first. My experience in writing 2/4 was that I indeed hit warnings through all four sub-commands. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/10/20 5:57 AM, Kashyap Chamarthy wrote: On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: [...] +qemu-img backing file without format (since 5.0.0) +'' + +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img +convert``, or ``qemu-img amend`` to create or modify an image that +depends on a backing file now recommends that an explicit backing +format be provided. Also for the `qemu-img amend` case, I'm not clear if the following scenario ought to throw the warning: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 This particular amend is not changing the backing file, and since I did not add warnings on the opening of an existing unsafe image, it is silent. Instead, you need to test a case where amend provokes a path that would change the backing file (perhaps as simple as '-o backing_file=./base.raw'), while omitting a format for the new backing file string. $> echo $? 0 I'm just trying to work out the scenarios where the warnings ought to show up (for all the four cases: create, rebase, convert, amend). [...] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/10/20 4:47 AM, Kashyap Chamarthy wrote: To provoke the warning during convert, you'll have to also pass -B (or -o backing_file), without -o backing_fmt (since convert lacks the -F shorthand). Hmm, I tried the following way, but it doesn't provoke the warning: $> ~/build/qemu/qemu-img convert -B ./base.raw -O qcow2 overlay1.qcow2 flattened.qcow2 $> ~/build/qemu/qemu-img info flattened.qcow2 image: flattened.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false What am I missing? Rather, it looks like my patch is missing a warning on that path. I'll have to investigate, for v4. - - - Ah, didn't realize the inconsistency of 'convert' lacking the '-F' shorthand ... which reminds me, there are at least _three_ ways that I know of, to specify backing file format with 'create': $ qemu-img create -f qcow2 -o 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2 I'm wondering about the consistency of having all the above three supported for other operations too. Now I at least know 'convert' lacks the "-F". The -o forms (backing_file= and backing_fmt=) always work. Various commands then have additional shorthand: -b/-F for create, -B for convert. You're right that we aren't very consistent, but I'm reluctant to change the inconsistencies in this patch (at one point in the past, we tried to get rid of the shorthand and force all users to go through -o, but that broke too many clients that were depending on the undocumented shorthand, so we documented the existing shorthand instead). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: [...] > > > +qemu-img backing file without format (since 5.0.0) > > > +'' > > > + > > > +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img > > > +convert``, or ``qemu-img amend`` to create or modify an image that > > > +depends on a backing file now recommends that an explicit backing > > > +format be provided. Also for the `qemu-img amend` case, I'm not clear if the following scenario ought to throw the warning: $> ~/build/qemu/qemu-img info --backing-chain overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB $> ~/build/qemu/qemu-img amend -o compat=v3 overlay1.qcow2 $> echo $? 0 I'm just trying to work out the scenarios where the warnings ought to show up (for all the four cases: create, rebase, convert, amend). [...] -- /kashyap
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Mon, Mar 09, 2020 at 10:42:25AM -0500, Eric Blake wrote: > On 3/9/20 10:31 AM, Kashyap Chamarthy wrote: > > > After (with the patch series applied to QEMU Git): > > > > $> git describe > > v4.2.0-2204-gd6c7830114 > > > > # Create; *without* specifying "-F raw" > > $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2 > > qemu-img: warning: Deprecated use of backing file without explicit > > backing format (detected format of raw) > > Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 > > backing_file=./base.raw backing_fmt=raw cluster_size=65536 > > lazy_refcounts=off refcount_bits=16 > > If you'll note, this case _did_ write an implied backing_fmt=raw into the > image. Ah, I missed to notice that. Interesting. > Constrast that with creating an image on a qcow2 backing file, which > tells you it detected a format of qcow2, but does NOT write > backing_fmt=qcow2 into the image (this was a change from v2, at Peter's > request). Indeed, here we go, confirming the overlay of a QCOW2 backing file _not_ having the 'backing_fmt' written into the image: $> ~/build/qemu/qemu-img create -f qcow2 -b ./another_base.qcow2 ./overlay1_of_ab.qcow2 qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of qcow2) Formatting './overlay1_of_ab.qcow2', fmt=qcow2 size=4294967296 backing_file=./another_base.qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16 That's neat. (I wonder if this bit should also be documented.) > Thus, when the backing is raw, we warn but future use of the > image is now safe where it previously was not; when the backing file is > non-raw, we warn but do not change our behavior, but because the backing > file is non-raw any future probes will not be any less safe than before. Understood. Easy to miss when not paying attention; thanks for pointing it out. [...] > > However, for the "Convert" case, is it correct that no warning is thrown > > for the below? > > > > $> ~/build/qemu/qemu-img info overlay1.qcow2 > > image: overlay1.qcow2 > > file format: qcow2 > > virtual size: 4 GiB (4294967296 bytes) > > disk size: 196 KiB > > cluster_size: 65536 > > backing file: base.raw > > Format specific information: > > compat: 1.1 > > lazy refcounts: false > > refcount bits: 16 > > corrupt: false > > We have an image with no backing format, so we had to probe. This patch > series did not change the behavior of opening an existing image, only for > creating a new image (or amending an image in-place). So the lack of a > warning on opening the unsafe image may be desirable, but it would be via > even more patches. Fair enough; it's an understandable compromise. And your series is a strict improvement as-is; it should not be held up. > > > $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 > > flattened.raw > > Ouch - you are creating a qcow2 destination file named 'flattened.raw', > which is rather confusing on your part. Oops, yes it is; bad me. Sorry for the mix-up. I meant to amend the format to 'raw'. > However, as your destination file is being created without a backing image, > it is to be expected that there is no warning (when there is no backing > file, -F makes no sense). Yeah, fair enough. > To provoke the warning during convert, you'll > have to also pass -B (or -o backing_file), without -o backing_fmt (since > convert lacks the -F shorthand). Hmm, I tried the following way, but it doesn't provoke the warning: $> ~/build/qemu/qemu-img convert -B ./base.raw -O qcow2 overlay1.qcow2 flattened.qcow2 $> ~/build/qemu/qemu-img info flattened.qcow2 image: flattened.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false What am I missing? - - - Ah, didn't realize the inconsistency of 'convert' lacking the '-F' shorthand ... which reminds me, there are at least _three_ ways that I know of, to specify backing file format with 'create': $ qemu-img create -f qcow2 -o 'backing_file=./base.raw,backing_fmt=raw' ./overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -o backing_fmt=raw overlay1.qcow2 $ qemu-img create -f qcow2 -b ./base.raw -F raw ./overlay1.qcow2 I'm wondering about the consistency of having all the above three supported for other operations too. Now I at least know 'convert' lacks the "-F". Not sure how much people care about this :-) [...] -- /kashyap
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On 3/9/20 10:31 AM, Kashyap Chamarthy wrote: After (with the patch series applied to QEMU Git): $> git describe v4.2.0-2204-gd6c7830114 # Create; *without* specifying "-F raw" $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2 qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of raw) Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16 If you'll note, this case _did_ write an implied backing_fmt=raw into the image. Constrast that with creating an image on a qcow2 backing file, which tells you it detected a format of qcow2, but does NOT write backing_fmt=qcow2 into the image (this was a change from v2, at Peter's request). Thus, when the backing is raw, we warn but future use of the image is now safe where it previously was not; when the backing file is non-raw, we warn but do not change our behavior, but because the backing file is non-raw any future probes will not be any less safe than before. # Rebase; *without* specifying "-F raw" $> ~/build/qemu/qemu-img rebase -b base.raw overlay1.qcow2 qemu-img: warning: Deprecated use of backing file without explicit backing format, use of this image requires potentially unsafe format probing However, for the "Convert" case, is it correct that no warning is thrown for the below? $> ~/build/qemu/qemu-img info overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false We have an image with no backing format, so we had to probe. This patch series did not change the behavior of opening an existing image, only for creating a new image (or amending an image in-place). So the lack of a warning on opening the unsafe image may be desirable, but it would be via even more patches. $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 flattened.raw Ouch - you are creating a qcow2 destination file named 'flattened.raw', which is rather confusing on your part. However, as your destination file is being created without a backing image, it is to be expected that there is no warning (when there is no backing file, -F makes no sense). To provoke the warning during convert, you'll have to also pass -B (or -o backing_file), without -o backing_fmt (since convert lacks the -F shorthand). $> echo $? 0 diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 6c1d9034d9e3..a8ffacf54a52 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -376,6 +376,25 @@ The above, converted to the current supported format:: Related binaries +qemu-img backing file without format (since 5.0.0) +'' + +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img +convert``, or ``qemu-img amend`` to create or modify an image that +depends on a backing file now recommends that an explicit backing +format be provided. This is for safety: if qemu probes a different +format than what you thought, the data presented to the guest will be +corrupt; similarly, presenting a raw image to a guest allows a +potential security exploit if a future probe sees a non-raw image +based on guest writes. To avoid the warning message, or even future +refusal to create an unsafe image, you must pass ``-o backing_fmt=`` +(or the shorthand ``-F`` during create) to specify the intended +backing format. You may use ``qemu-img rebase -u`` to retroactively +add a backing format to an existing image. However, be aware that +there are already potential security risks to blindly using ``qemu-img +info`` to probe the format of an untrusted backing image, when +deciding what format to add into an existing image. Nit: s/qemu/QEMU/g/ Ultra Nit: should this paragraph be broken down into two? Experience tells people usually feel deterred read "substantial paragraphs" :-) Could do, right before 'To avoid the warning'. I'll report back the Amend case. (And once I get clarification on the Convert scenario, I'll be happy to give Tested-by.) [...] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
On Fri, Mar 06, 2020 at 04:51:21PM -0600, Eric Blake wrote: > Creating an image that requires format probing of the backing image is > inherently unsafe (we've had several CVEs over the years based on > probes leaking information to the guest on a subsequent boot, although > these days tools like libvirt are aware of the issue enough to prevent > the worst effects). However, if our probing algorithm ever changes, > or if other tools like libvirt determine a different probe result than > we do, then subsequent use of that backing file under a different > format will present corrupted data to the guest. Start a deprecation > clock so that future qemu-img can refuse to create unsafe backing > chains that would rely on probing. The warnings are intentionally > emitted from the block layer rather than qemu-img (thus, all paths > into image creation or rewriting perform the check). I happily welcome this change. I'm going around and fixing various docs of differnt projects that create overlays without explicitly spelling out backing files. (FWIW, I also make sure to mention this, in context, in all QEMU-related talks I give publicliy.) This proactive action from QEMU will definitely help. > However, there is one time where probing is safe: if we probe raw, > then it is safe to record that implicitly in the image (but we still > warn, as it's better to teach the user to supply -F always than to > make them guess when it is safe). > > iotest 114 specifically wants to create an unsafe image for later > amendment rather than defaulting to our new default of recording a > probed format, so it needs an update. While touching it, expand it to > cover all of the various warnings enabled by this patch. > > Signed-off-by: Eric Blake > --- > docs/system/deprecated.rst | 19 +++ > block.c| 21 - > qemu-img.c | 2 +- > tests/qemu-iotests/114 | 11 +++ > tests/qemu-iotests/114.out | 8 > 5 files changed, 59 insertions(+), 2 deletions(-) Before (with: qemu-4.2.0-2.fc30): $> qemu-img create -f qcow2 -b ./base.raw ./overlay1.qcow2 Formatting './overlay1.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw cluster_size=65536 lazy_refcounts=off refcount_bits=16 After (with the patch series applied to QEMU Git): $> git describe v4.2.0-2204-gd6c7830114 # Create; *without* specifying "-F raw" $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2 qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of raw) Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16 # Rebase; *without* specifying "-F raw" $> ~/build/qemu/qemu-img rebase -b base.raw overlay1.qcow2 qemu-img: warning: Deprecated use of backing file without explicit backing format, use of this image requires potentially unsafe format probing However, for the "Convert" case, is it correct that no warning is thrown for the below? $> ~/build/qemu/qemu-img info overlay1.qcow2 image: overlay1.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 flattened.raw $> echo $? 0 > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index 6c1d9034d9e3..a8ffacf54a52 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -376,6 +376,25 @@ The above, converted to the current supported format:: > Related binaries > > > +qemu-img backing file without format (since 5.0.0) > +'' > + > +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img > +convert``, or ``qemu-img amend`` to create or modify an image that > +depends on a backing file now recommends that an explicit backing > +format be provided. This is for safety: if qemu probes a different > +format than what you thought, the data presented to the guest will be > +corrupt; similarly, presenting a raw image to a guest allows a > +potential security exploit if a future probe sees a non-raw image > +based on guest writes. To avoid the warning message, or even future > +refusal to create an unsafe image, you must pass ``-o backing_fmt=`` > +(or the shorthand ``-F`` during create) to specify the intended > +backing format. You may use ``qemu-img rebase -u`` to retroactively > +add a backing format to an existing image. However, be aware that > +there are already potential security risks to blindly using ``qemu-img > +info`` to probe the format of an untrusted backing image, when
[PATCH v3 4/4] qemu-img: Deprecate use of -b without -F
Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot, although
these days tools like libvirt are aware of the issue enough to prevent
the worst effects). However, if our probing algorithm ever changes,
or if other tools like libvirt determine a different probe result than
we do, then subsequent use of that backing file under a different
format will present corrupted data to the guest. Start a deprecation
clock so that future qemu-img can refuse to create unsafe backing
chains that would rely on probing. The warnings are intentionally
emitted from the block layer rather than qemu-img (thus, all paths
into image creation or rewriting perform the check).
However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).
iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update. While touching it, expand it to
cover all of the various warnings enabled by this patch.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 19 +++
block.c| 21 -
qemu-img.c | 2 +-
tests/qemu-iotests/114 | 11 +++
tests/qemu-iotests/114.out | 8
5 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 6c1d9034d9e3..a8ffacf54a52 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -376,6 +376,25 @@ The above, converted to the current supported format::
Related binaries
+qemu-img backing file without format (since 5.0.0)
+''
+
+The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
+convert``, or ``qemu-img amend`` to create or modify an image that
+depends on a backing file now recommends that an explicit backing
+format be provided. This is for safety: if qemu probes a different
+format than what you thought, the data presented to the guest will be
+corrupt; similarly, presenting a raw image to a guest allows a
+potential security exploit if a future probe sees a non-raw image
+based on guest writes. To avoid the warning message, or even future
+refusal to create an unsafe image, you must pass ``-o backing_fmt=``
+(or the shorthand ``-F`` during create) to specify the intended
+backing format. You may use ``qemu-img rebase -u`` to retroactively
+add a backing format to an existing image. However, be aware that
+there are already potential security risks to blindly using ``qemu-img
+info`` to probe the format of an untrusted backing image, when
+deciding what format to add into an existing image.
+
``qemu-img convert -n -o`` (since 4.2.0)
diff --git a/block.c b/block.c
index 43452976acdc..ad49d515809c 100644
--- a/block.c
+++ b/block.c
@@ -6039,6 +6039,20 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"Could not open backing image to determine
size.\n");
goto out;
} else {
+if (!backing_fmt) {
+warn_report("Deprecated use of backing file without explicit "
+"backing format (detected format of %s)",
+bs->drv->format_name);
+if (bs->drv == _raw) {
+/*
+ * A probe of raw is always correct, so in this one
+ * case, we can write that into the image.
+ */
+backing_fmt = bs->drv->format_name;
+qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt,
+ NULL);
+}
+}
if (size == -1) {
/* Opened BS, have no size */
size = bdrv_getlength(bs);
@@ -6052,7 +6066,12 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
bdrv_unref(bs);
}
-} /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+/* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+} else if (backing_file && !backing_fmt) {
+warn_report("Deprecated use of unopened backing file without "
+"explicit backing format, use of this image requires "
+"potentially unsafe format probing");
+}
if (size == -1) {
error_setg(errp, "Image creation needs a size parameter");
diff --git a/qemu-img.c b/qemu-img.c
index b9375427404d..48424f8dbcd4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3637,7 +3637,7 @@ static int img_rebase(int argc, char
