block: Don't probe for unknown backing file format

PATCH Applied Kevin Wolf 3 participants 9 messages Nov 25, 2014 – Dec 2, 2014 Block layer

Share & tools

Kevin Wolf

author

2014-11-25 17:13 UTC #0 lore raw

v2 cover block: Don't probe for unknown backing file format

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

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-25 17:13 UTC #1 lore raw

PATCH v2 1/3 qcow2: Fix header extension size check

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 [email protected]

Reported-by Max Reitz <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2.c              | 2 +-
 tests/qemu-iotests/080     | 2 ++
 tests/qemu-iotests/080.out | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

block/qcow2.c +1 -1

index d120494..8b9ffc4 100644
@@ -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;
         }

tests/qemu-iotests/080 +2 -0

index 9de337c..73795f1 100755
@@ -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

tests/qemu-iotests/080.out +2 -0

index f7a943c..33d1f71 100644
@@ -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

Signature · 1 line

1.8.3.1

Max Reitz

Reviewed-by on v2 1/3

2014-11-25 17:23 UTC #2 lore raw

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

Quoted 7 lines · 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.

Quoted 8 lines

Cc [email protected]
Reported-by Max Reitz <[email protected]>
Signed-off-by Kevin Wolf <[email protected]>
---
  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 <[email protected]>

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

Kevin Wolf

author

2014-11-25 17:13 UTC #3 lore raw

PATCH v2 2/3 qcow2.py: Add required padding for header extensions

In reply to 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 <[email protected]>

---
 tests/qemu-iotests/qcow2.py | 4 ++++
 1 file changed, 4 insertions(+)

tests/qemu-iotests/qcow2.py +4 -0

index 2058596..9cc4cf7 100755
@@ -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

Signature · 1 line

1.8.3.1

Max Reitz

Reviewed-by on v2 2/3

2014-11-25 17:24 UTC #4 lore raw

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

Quoted 13 lines · 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 <[email protected]>
---
  tests/qemu-iotests/qcow2.py | 4 ++++
  1 file changed, 4 insertions(+)
tests/qemu-iotests/qcow2.py +0 -0
index 2058596..9cc4cf7 100755

tests/qemu-iotests/qcow2.py import string · line 12

Quoted 6 lines

tests/qemu-iotests/qcow2.py +3 -0

@@ -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*

tests/qemu-iotests/qcow2.py import string · line 16

Quoted 4 lines

tests/qemu-iotests/qcow2.py +1 -0

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

Reviewed-by Max Reitz <[email protected]>

Kevin Wolf

author

2014-11-25 17:13 UTC #5 lore raw

PATCH v2 3/3 block: Don't probe for unknown backing file format

In reply to 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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

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

block.c +3 -4

index ded9bce..82d6dfb 100644
@@ -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;

tests/qemu-iotests/114 new file +61 -0

61 lines not on this page · raw message

tests/qemu-iotests/114.out new file +13 -0

new file mode 100644
index 0000000..6c6b210
@@ -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

tests/qemu-iotests/group +1 -0

index 7dfe469..9188049 100644
@@ -112,3 +112,4 @@
 107 rw auto quick
 108 rw auto quick
 111 rw auto quick
+114 rw auto quick

Signature · 1 line

1.8.3.1

Max Reitz

Reviewed-by on v2 3/3

2014-11-25 17:25 UTC #6 lore raw

Quoted 11 lines · 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 [email protected]
Signed-off-by Kevin Wolf <[email protected]>

Quoted 8 lines

---
  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 <[email protected]>

Stefan Hajnoczi

Applied

2014-11-28 11:33 UTC #7 lore raw

In reply to Kevin Wolf

Quoted 10 lines · 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

Quoted 16 lines

 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

Signature · 1 line

1.8.3.1

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

Stefan

Kevin Wolf

author

2014-12-02 10:23 UTC #8 lore raw

Am 28.11.2014 um 12:33 hat Stefan Hajnoczi geschrieben:

Quoted 23 lines · Kevin Wolf

Quoted 22 lines
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

Quoted 7 lines

Quoted 4 lines
Signature · 1 line
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