[Qemu-devel] [PATCH v2 0/3] block: Don't probe for unknown backing file format

[Qemu-devel] [PATCH v2 0/3] block: Don't probe for unknown backing file format

[Kevin Wolf]

v2:
- Added two patches to make the test case actually work properly and not
  just by accident on my laptop. [Max]
- Fixed comment in test case [Max]

Kevin Wolf (3):
  qcow2: Fix header extension size check
  qcow2.py: Add required padding for header extensions
  block: Don't probe for unknown backing file format

 block.c                     |  7 +++---
 block/qcow2.c               |  2 +-
 tests/qemu-iotests/080      |  2 ++
 tests/qemu-iotests/080.out  |  2 ++
 tests/qemu-iotests/114      | 61 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/114.out  | 13 ++++++++++
 tests/qemu-iotests/group    |  1 +
 tests/qemu-iotests/qcow2.py |  4 +++
 8 files changed, 87 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/114
 create mode 100644 tests/qemu-iotests/114.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/3] qcow2: Fix header extension size check

[Kevin Wolf]

After reading the extension header, offset is incremented, but not
checked against end_offset any more. This way an integer overflow could
happen when checking whether the extension end is within the allowed
range, effectively disabling the check.

This patch adds the missing check and a test case for it.

Cc: qemu-stable@nongnu.org
Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c              | 2 +-
 tests/qemu-iotests/080     | 2 ++
 tests/qemu-iotests/080.out | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d120494..8b9ffc4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -117,7 +117,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
 #ifdef DEBUG_EXT
         printf("ext.magic = 0x%x\n", ext.magic);
 #endif
-        if (ext.len > end_offset - offset) {
+        if (offset > end_offset || ext.len > end_offset - offset) {
             error_setg(errp, "Header extension too large");
             return -EINVAL;
         }
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 9de337c..73795f1 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -78,6 +78,8 @@ poke_file "$TEST_IMG" "$offset_backing_file_offset" "\xff\xff\xff\xff\xff\xff\xf
 poke_file "$TEST_IMG" "$offset_ext_magic" "\x12\x34\x56\x78"
 poke_file "$TEST_IMG" "$offset_ext_size" "\x7f\xff\xff\xff"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_backing_file_offset" "\x00\x00\x00\x00\x00\x00\x00\x$(printf %x $offset_ext_size)"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 poke_file "$TEST_IMG" "$offset_backing_file_offset" "\x00\x00\x00\x00\x00\x00\x00\x00"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index f7a943c..33d1f71 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -13,6 +13,8 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Invalid backing file offset
 no file open, try 'help open'
 qemu-io: can't open device TEST_DIR/t.qcow2: Header extension too large
 no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow2: Header extension too large
+no file open, try 'help open'
 
 == Huge refcount table size ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/3] qcow2: Fix header extension size check

[Max Reitz]

On 2014-11-25 at 18:12, Kevin Wolf wrote:
> After reading the extension header, offset is incremented, but not
> checked against end_offset any more. This way an integer overflow could
> happen when checking whether the extension end is within the allowed
> range, effectively disabling the check.
>
> This patch adds the missing check and a test case for it.
>
> Cc: qemu-stable@nongnu.org
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c              | 2 +-
>   tests/qemu-iotests/080     | 2 ++
>   tests/qemu-iotests/080.out | 2 ++
>   3 files changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Using g_try_malloc0() might have been nice anyway, but it should work 
without, now (although it should have worked before as well...).

[Qemu-devel] [PATCH v2 2/3] qcow2.py: Add required padding for header extensions

[Kevin Wolf]

The qcow2 specification requires that the header extension data be
padded to round up the extension size to the next multiple of 8 bytes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/qcow2.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 2058596..9cc4cf7 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -7,6 +7,10 @@ import string
 class QcowHeaderExtension:
 
     def __init__(self, magic, length, data):
+        if length % 8 != 0:
+            padding = 8 - (length % 8)
+            data += "\0" * padding
+
         self.magic  = magic
         self.length = length
         self.data   = data
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 2/3] qcow2.py: Add required padding for header extensions

[Max Reitz]

On 2014-11-25 at 18:12, Kevin Wolf wrote:
> The qcow2 specification requires that the header extension data be
> padded to round up the extension size to the next multiple of 8 bytes.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/qcow2.py | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
> index 2058596..9cc4cf7 100755
> --- a/tests/qemu-iotests/qcow2.py
> +++ b/tests/qemu-iotests/qcow2.py
> @@ -7,6 +7,10 @@ import string
>   class QcowHeaderExtension:
>   
>       def __init__(self, magic, length, data):
> +        if length % 8 != 0:
> +            padding = 8 - (length % 8)
> +            data += "\0" * padding

So it does work that way, great. *g*

> +
>           self.magic  = magic
>           self.length = length
>           self.data   = data

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v2 3/3] block: Don't probe for unknown backing file format

[Kevin Wolf]

If a qcow2 image specifies a backing file format that doesn't correspond
to any format driver that qemu knows, we shouldn't fall back to probing,
but simply error out.

Not looking up the backing file driver in bdrv_open_backing_file(), but
just filling in the "driver" option if it isn't there moves us closer to
the goal of having everything in QDict options and gets us the error
handling of bdrv_open(), which correctly refuses unknown drivers.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                    |  7 +++---
 tests/qemu-iotests/114     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/114.out | 13 ++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100755 tests/qemu-iotests/114
 create mode 100644 tests/qemu-iotests/114.out

diff --git a/block.c b/block.c
index ded9bce..82d6dfb 100644
--- a/block.c
+++ b/block.c
@@ -1180,7 +1180,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
 {
     char *backing_filename = g_malloc0(PATH_MAX);
     int ret = 0;
-    BlockDriver *back_drv = NULL;
     BlockDriverState *backing_hd;
     Error *local_err = NULL;
 
@@ -1213,14 +1212,14 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
 
     backing_hd = bdrv_new();
 
-    if (bs->backing_format[0] != '\0') {
-        back_drv = bdrv_find_format(bs->backing_format);
+    if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
+        qdict_put(options, "driver", qstring_from_str(bs->backing_format));
     }
 
     assert(bs->backing_hd == NULL);
     ret = bdrv_open(&backing_hd,
                     *backing_filename ? backing_filename : NULL, NULL, options,
-                    bdrv_backing_flags(bs->open_flags), back_drv, &local_err);
+                    bdrv_backing_flags(bs->open_flags), NULL, &local_err);
     if (ret < 0) {
         bdrv_unref(backing_hd);
         backing_hd = NULL;
diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
new file mode 100755
index 0000000..d02e7ff
--- /dev/null
+++ b/tests/qemu-iotests/114
@@ -0,0 +1,61 @@
+#!/bin/bash
+#
+# Test invalid backing file format in qcow2 images
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+TEST_IMG="$TEST_IMG.base" _make_test_img 64M
+_make_test_img -b "$TEST_IMG.base" 64M
+
+# Set an invalid backing file format
+$PYTHON qcow2.py "$TEST_IMG" add-header-ext 0xE2792ACA "foo"
+_img_info
+
+# Try opening the image. Should fail (and not probe) in the first case, but
+# overriding the backing file format should be possible.
+$QEMU_IO -c "open $TEST_IMG" -c "read 0 4k" 2>&1 | _filter_qemu_io | _filter_testdir
+$QEMU_IO -c "open -o backing.driver=$IMGFMT $TEST_IMG" -c "read 0 4k" | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out
new file mode 100644
index 0000000..6c6b210
--- /dev/null
+++ b/tests/qemu-iotests/114.out
@@ -0,0 +1,13 @@
+QA output created by 114
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='TEST_DIR/t.IMGFMT.base' 
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: foo
+qemu-io: can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown driver 'foo'
+read 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7dfe469..9188049 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -112,3 +112,4 @@
 107 rw auto quick
 108 rw auto quick
 111 rw auto quick
+114 rw auto quick
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 3/3] block: Don't probe for unknown backing file format

[Max Reitz]

On 2014-11-25 at 18:12, Kevin Wolf wrote:
> If a qcow2 image specifies a backing file format that doesn't correspond
> to any format driver that qemu knows, we shouldn't fall back to probing,
> but simply error out.
>
> Not looking up the backing file driver in bdrv_open_backing_file(), but
> just filling in the "driver" option if it isn't there moves us closer to
> the goal of having everything in QDict options and gets us the error
> handling of bdrv_open(), which correctly refuses unknown drivers.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c                    |  7 +++---
>   tests/qemu-iotests/114     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/114.out | 13 ++++++++++
>   tests/qemu-iotests/group   |  1 +
>   4 files changed, 78 insertions(+), 4 deletions(-)
>   create mode 100755 tests/qemu-iotests/114
>   create mode 100644 tests/qemu-iotests/114.out

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/3] block: Don't probe for unknown backing file format

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Tue, Nov 25, 2014 at 06:12:39PM +0100, Kevin Wolf wrote:
> v2:
> - Added two patches to make the test case actually work properly and not
>   just by accident on my laptop. [Max]
> - Fixed comment in test case [Max]
> 
> Kevin Wolf (3):
>   qcow2: Fix header extension size check
>   qcow2.py: Add required padding for header extensions
>   block: Don't probe for unknown backing file format
> 
>  block.c                     |  7 +++---
>  block/qcow2.c               |  2 +-
>  tests/qemu-iotests/080      |  2 ++
>  tests/qemu-iotests/080.out  |  2 ++
>  tests/qemu-iotests/114      | 61 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/114.out  | 13 ++++++++++
>  tests/qemu-iotests/group    |  1 +
>  tests/qemu-iotests/qcow2.py |  4 +++
>  8 files changed, 87 insertions(+), 5 deletions(-)
>  create mode 100755 tests/qemu-iotests/114
>  create mode 100644 tests/qemu-iotests/114.out
> 
> -- 
> 1.8.3.1
> 
> 

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 0/3] block: Don't probe for unknown backing file format

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

Am 28.11.2014 um 12:33 hat Stefan Hajnoczi geschrieben:
> On Tue, Nov 25, 2014 at 06:12:39PM +0100, Kevin Wolf wrote:
> > v2:
> > - Added two patches to make the test case actually work properly and not
> >   just by accident on my laptop. [Max]
> > - Fixed comment in test case [Max]
> > 
> > Kevin Wolf (3):
> >   qcow2: Fix header extension size check
> >   qcow2.py: Add required padding for header extensions
> >   block: Don't probe for unknown backing file format
> > 
> >  block.c                     |  7 +++---
> >  block/qcow2.c               |  2 +-
> >  tests/qemu-iotests/080      |  2 ++
> >  tests/qemu-iotests/080.out  |  2 ++
> >  tests/qemu-iotests/114      | 61 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/114.out  | 13 ++++++++++
> >  tests/qemu-iotests/group    |  1 +
> >  tests/qemu-iotests/qcow2.py |  4 +++
> >  8 files changed, 87 insertions(+), 5 deletions(-)
> >  create mode 100755 tests/qemu-iotests/114
> >  create mode 100644 tests/qemu-iotests/114.out
> > 
> > -- 
> > 1.8.3.1
> > 
> > 
> 
> Thanks, applied to my block-next tree:
> https://github.com/stefanha/qemu/commits/block-next

Somehow you lost the new files (114 and 114.out) in patch 3. I'm fixing
this up in my block-next branch now, but I thought I should tell you in
case you need to fix some local script.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]