[Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Levente Kurusa]

Fixed size VPC images do not have a footer, hence the current probe
function will fail and QEMU will fall back to the raw_bsd driver, which is
not the correct behaviour. The specification of the format says that fixed
size images have a footer as the last 512 bytes of the file. The footer is
exactly the same as the header would be in the case of dynamically growing
images.

For this, we need to read the last 512 bytes of the image, however the
current mechanics predominantly read the first 2048 bytes and pass that
as a buffer to the probe functions. Solve this by passing the
BlockDriverState to the probe functions, hence giving them a chance to read
the extra bytes they might need.

Levente Kurusa (3):
  block: format: pass down the current state to the format's probe
    function
  block: vpc: introduce vpc_check_signature function
  block: vpc: handle fixed size images in probe function

 block.c                   |  2 +-
 block/bochs.c             |  3 ++-
 block/cloop.c             |  3 ++-
 block/cow.c               |  3 ++-
 block/dmg.c               |  3 ++-
 block/parallels.c         |  3 ++-
 block/qcow.c              |  3 ++-
 block/qcow2.c             |  3 ++-
 block/qed.c               |  4 ++--
 block/raw_bsd.c           |  3 ++-
 block/vdi.c               |  3 ++-
 block/vmdk.c              |  3 ++-
 block/vpc.c               | 34 +++++++++++++++++++++++++++++-----
 include/block/block_int.h |  3 ++-
 14 files changed, 54 insertions(+), 19 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] block: format: pass down the current state to the format's probe function

[Levente Kurusa]

While most ->probe functions are content with reading the first 2048
bytes of their disk, there are some (namely VPC) which might need to
know more about the disk. Pass down the BlockDriverState to the probe
functions, so that the drivers can read more data should they need it.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Levente Kurusa <lkurusa@redhat.com>
---
 block.c                   | 2 +-
 block/bochs.c             | 3 ++-
 block/cloop.c             | 3 ++-
 block/cow.c               | 3 ++-
 block/dmg.c               | 3 ++-
 block/parallels.c         | 3 ++-
 block/qcow.c              | 3 ++-
 block/qcow2.c             | 3 ++-
 block/qed.c               | 4 ++--
 block/raw_bsd.c           | 3 ++-
 block/vdi.c               | 3 ++-
 block/vmdk.c              | 3 ++-
 block/vpc.c               | 3 ++-
 include/block/block_int.h | 3 ++-
 14 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 8cf519b..90dd8f0 100644
--- a/block.c
+++ b/block.c
@@ -683,7 +683,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
     drv = NULL;
     QLIST_FOREACH(drv1, &bdrv_drivers, list) {
         if (drv1->bdrv_probe) {
-            score = drv1->bdrv_probe(buf, ret, filename);
+            score = drv1->bdrv_probe(bs, buf, ret, filename);
             if (score > score_max) {
                 score_max = score;
                 drv = drv1;
diff --git a/block/bochs.c b/block/bochs.c
index eba23df..9e445f4 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -76,7 +76,8 @@ typedef struct BDRVBochsState {
     uint32_t extent_size;
 } BDRVBochsState;
 
-static int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int bochs_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                       const char *filename)
 {
     const struct bochs_header *bochs = (const void *)buf;
 
diff --git a/block/cloop.c b/block/cloop.c
index 8457737..f707699 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -41,7 +41,8 @@ typedef struct BDRVCloopState {
     z_stream zstream;
 } BDRVCloopState;
 
-static int cloop_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int cloop_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                       const char *filename)
 {
     const char *magic_version_2_0 = "#!/bin/sh\n"
         "#V2.0 Format\n"
diff --git a/block/cow.c b/block/cow.c
index 6ee4833..6002c62 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -46,7 +46,8 @@ typedef struct BDRVCowState {
     int64_t cow_sectors_offset;
 } BDRVCowState;
 
-static int cow_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int cow_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                     const char *filename)
 {
     const struct cow_header_v2 *cow_header = (const void *)buf;
 
diff --git a/block/dmg.c b/block/dmg.c
index 1e153cd..962c4fc 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -57,7 +57,8 @@ typedef struct BDRVDMGState {
     z_stream zstream;
 } BDRVDMGState;
 
-static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int dmg_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                     const char *filename)
 {
     int len;
 
diff --git a/block/parallels.c b/block/parallels.c
index 1a5bd35..157d0a0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -54,7 +54,8 @@ typedef struct BDRVParallelsState {
     unsigned int tracks;
 } BDRVParallelsState;
 
-static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int parallels_probe(BlockDriverState *bs, const uint8_t *buf,
+                           int buf_size, const char *filename)
 {
     const struct parallels_header *ph = (const void *)buf;
 
diff --git a/block/qcow.c b/block/qcow.c
index a874056..acd126a 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -81,7 +81,8 @@ typedef struct BDRVQcowState {
 
 static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
 
-static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int qcow_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                      const char *filename)
 {
     const QCowHeader *cow_header = (const void *)buf;
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 1e3ab6b..14593db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -59,7 +59,8 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
 
-static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int qcow2_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                       const char *filename)
 {
     const QCowHeader *cow_header = (const void *)buf;
 
diff --git a/block/qed.c b/block/qed.c
index 7944832..7b492e9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -36,8 +36,8 @@ static const AIOCBInfo qed_aiocb_info = {
     .cancel             = qed_aio_cancel,
 };
 
-static int bdrv_qed_probe(const uint8_t *buf, int buf_size,
-                          const char *filename)
+static int bdrv_qed_probe(BlockDriverState *bs, const uint8_t *buf,
+                          int buf_size, const char *filename)
 {
     const QEDHeader *header = (const QEDHeader *)buf;
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index f82f4c2..0a2b45f 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -165,7 +165,8 @@ static void raw_close(BlockDriverState *bs)
 {
 }
 
-static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int raw_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                     const char *filename)
 {
     /* smallest possible positive score so that raw is used if and only if no
      * other block driver works
diff --git a/block/vdi.c b/block/vdi.c
index 197bd77..a30ccb1 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -354,7 +354,8 @@ static int vdi_make_empty(BlockDriverState *bs)
     return 0;
 }
 
-static int vdi_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int vdi_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                     const char *filename)
 {
     const VdiHeader *header = (const VdiHeader *)buf;
     int result = 0;
diff --git a/block/vmdk.c b/block/vmdk.c
index 0517bba..8302876 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -145,7 +145,8 @@ enum {
     MARKER_FOOTER           = 3,
 };
 
-static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int vmdk_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                      const char *filename)
 {
     uint32_t magic;
 
diff --git a/block/vpc.c b/block/vpc.c
index 8b376a4..2ba8fc2 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -157,7 +157,8 @@ static uint32_t vpc_checksum(uint8_t* buf, size_t size)
 }
 
 
-static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int vpc_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
+                     const char *filename)
 {
     if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
 	return 100;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7b541a0..2825cef 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -88,7 +88,8 @@ struct BlockDriver {
     bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
                                              BlockDriverState *candidate);
 
-    int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
+    int (*bdrv_probe)(BlockDriverState *bs, const uint8_t *buf,
+                      int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
 
     /* Any driver implementing this callback is expected to be able to handle
-- 
1.9.3

[Qemu-devel] [PATCH 2/3] block: vpc: introduce vpc_check_signature function

[Levente Kurusa]

Check the signature in a helper function instead of in plain
code in order to avoid code duplication

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Levente Kurusa <lkurusa@redhat.com>
---
 block/vpc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 2ba8fc2..a6a7213 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -156,11 +156,15 @@ static uint32_t vpc_checksum(uint8_t* buf, size_t size)
     return ~res;
 }
 
+static int vpc_check_signature(const void *buf)
+{
+    return !strncmp((char *)buf, "conectix", 8);
+}
 
 static int vpc_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
                      const char *filename)
 {
-    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
+    if (buf_size >= 8 && vpc_check_signature(buf))
 	return 100;
     return 0;
 }
@@ -184,7 +188,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     footer = (VHDFooter *) s->footer_buf;
-    if (strncmp(footer->creator, "conectix", 8)) {
+    if (!vpc_check_signature(footer->creator)) {
         int64_t offset = bdrv_getlength(bs->file);
         if (offset < 0) {
             ret = offset;
@@ -200,7 +204,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto fail;
         }
-        if (strncmp(footer->creator, "conectix", 8)) {
+        if (!vpc_check_signature(footer->creator)) {
             error_setg(errp, "invalid VPC image");
             ret = -EINVAL;
             goto fail;
-- 
1.9.3

[Qemu-devel] [PATCH 3/3] block: vpc: handle fixed size images in probe function

[Levente Kurusa]

Fixed size images do not have a header, only dynamic images have that.
This type uses a footer, which is the same structure in the last 512
bytes of the image. We need to parse that too to be able to recognize
fixed length images, so check there as well.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Levente Kurusa <lkurusa@redhat.com>
---
 block/vpc.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index a6a7213..b12354a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -164,8 +164,27 @@ static int vpc_check_signature(const void *buf)
 static int vpc_probe(BlockDriverState *bs, const uint8_t *buf, int buf_size,
                      const char *filename)
 {
-    if (buf_size >= 8 && vpc_check_signature(buf))
-	return 100;
+    char sig[8];
+
+    if (buf_size < 8) {
+        return 0;
+    }
+
+    if (vpc_check_signature(buf)) {
+        return 100;
+    }
+
+    /*
+     * Don't give up just yet, since the spec say that only dynamic
+     * images have a header (which in fact is a copy of the footer).
+     * Check the signature in the footer as well in order to handle
+     * fixed size images.
+     */
+    buf_size = bdrv_pread(bs, bdrv_getlength(bs) - HEADER_SIZE, sig, 8);
+    if (buf_size >= 8 && vpc_check_signature(sig)) {
+        return 100;
+    }
+
     return 0;
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Stefan Hajnoczi]

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

On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> Fixed size VPC images do not have a footer, hence the current probe
> function will fail and QEMU will fall back to the raw_bsd driver, which is
> not the correct behaviour. The specification of the format says that fixed
> size images have a footer as the last 512 bytes of the file. The footer is
> exactly the same as the header would be in the case of dynamically growing
> images.
> 
> For this, we need to read the last 512 bytes of the image, however the
> current mechanics predominantly read the first 2048 bytes and pass that
> as a buffer to the probe functions. Solve this by passing the
> BlockDriverState to the probe functions, hence giving them a chance to read
> the extra bytes they might need.

I hesitate to add patches that extend image format probing.  For the
past few years we have always recommended that image files should not be
probed.

Image probing is prone to security issues because a malicious guest can
modify a raw or vpc image by putting another image format header at
sector 0.  The next time QEMU opens the image it will detect a different
format.  One evil trick is to refer to a file on the host file system as
the backing file, now you can read any file that the QEMU process has
access to.

Probing also complicates live migration.  The source host still has the
image file open and may write to it.  The destination host shouldn't
even read from the image file before handover to avoid file cache
coherency issues.

Probing is broken.  It shouldn't be used.  We shouldn't extend it
(especially by adding more I/Os).

QEMU has the explicit -drive format= option.  qemu-img has -F and -O
options to specify the format.

Can you use format=vpc?

Stefan

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

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> > Fixed size VPC images do not have a footer, hence the current probe
> > function will fail and QEMU will fall back to the raw_bsd driver, which is
> > not the correct behaviour. The specification of the format says that fixed
> > size images have a footer as the last 512 bytes of the file. The footer is
> > exactly the same as the header would be in the case of dynamically growing
> > images.
> > 
> > For this, we need to read the last 512 bytes of the image, however the
> > current mechanics predominantly read the first 2048 bytes and pass that
> > as a buffer to the probe functions. Solve this by passing the
> > BlockDriverState to the probe functions, hence giving them a chance to read
> > the extra bytes they might need.
> 
> I hesitate to add patches that extend image format probing.  For the
> past few years we have always recommended that image files should not be
> probed.
> 
> Image probing is prone to security issues because a malicious guest can
> modify a raw or vpc image by putting another image format header at
> sector 0.  The next time QEMU opens the image it will detect a different
> format.  One evil trick is to refer to a file on the host file system as
> the backing file, now you can read any file that the QEMU process has
> access to.
> 
> Probing also complicates live migration.  The source host still has the
> image file open and may write to it.  The destination host shouldn't
> even read from the image file before handover to avoid file cache
> coherency issues.
> 
> Probing is broken.  It shouldn't be used.  We shouldn't extend it
> (especially by adding more I/Os).
>

For 2.2, maybe we should limit probing to only certain operations (e.g.
qemu-img info) - or perhaps just remove the capability altogether, or
at least start phasing it out with a warning message that automatic
format detection is deprecated and may be unsafe.

Jeff

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Levente Kurusa]

On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
> On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> > > Fixed size VPC images do not have a footer, hence the current probe
> > > function will fail and QEMU will fall back to the raw_bsd driver, which
> > > is
> > > not the correct behaviour. The specification of the format says that
> > > fixed
> > > size images have a footer as the last 512 bytes of the file. The footer
> > > is
> > > exactly the same as the header would be in the case of dynamically
> > > growing
> > > images.
> > > 
> > > For this, we need to read the last 512 bytes of the image, however the
> > > current mechanics predominantly read the first 2048 bytes and pass that
> > > as a buffer to the probe functions. Solve this by passing the
> > > BlockDriverState to the probe functions, hence giving them a chance to
> > > read
> > > the extra bytes they might need.
> > 
> > I hesitate to add patches that extend image format probing.  For the
> > past few years we have always recommended that image files should not be
> > probed.
> > 
> > Image probing is prone to security issues because a malicious guest can
> > modify a raw or vpc image by putting another image format header at
> > sector 0.  The next time QEMU opens the image it will detect a different
> > format.  One evil trick is to refer to a file on the host file system as
> > the backing file, now you can read any file that the QEMU process has
> > access to.

Yea, good point. The current state of probing is actually quite bad,
just take a look at dmg_probe in block/dmg.c :-(

> > 
> > Probing also complicates live migration.  The source host still has the
> > image file open and may write to it.  The destination host shouldn't
> > even read from the image file before handover to avoid file cache
> > coherency issues.
> > 
> > Probing is broken.  It shouldn't be used.  We shouldn't extend it
> > (especially by adding more I/Os).

Even though, my series would have only added one extra I/O in the case
of failing VPC images, I have to admit you are right.

> 
> For 2.2, maybe we should limit probing to only certain operations (e.g.
> qemu-img info) - or perhaps just remove the capability altogether, or
> at least start phasing it out with a warning message that automatic
> format detection is deprecated and may be unsafe.
> 

Considering the fact that most open functions already check the magic
numbers, and they do a lot better/safer job at it, we could just swap
the probe functions with the open ones and just insert an fprintf
when format is not specified doing what Jeff suggested.

Any objections to this?

(This would also solve the VPC-fixed-size bug, since vpc_open already
checks the footer if the header is not found)

Thanks,
Levente Kurusa

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
> On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
> > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> > > > Fixed size VPC images do not have a footer, hence the current probe
> > > > function will fail and QEMU will fall back to the raw_bsd driver, which
> > > > is
> > > > not the correct behaviour. The specification of the format says that
> > > > fixed
> > > > size images have a footer as the last 512 bytes of the file. The footer
> > > > is
> > > > exactly the same as the header would be in the case of dynamically
> > > > growing
> > > > images.
> > > > 
> > > > For this, we need to read the last 512 bytes of the image, however the
> > > > current mechanics predominantly read the first 2048 bytes and pass that
> > > > as a buffer to the probe functions. Solve this by passing the
> > > > BlockDriverState to the probe functions, hence giving them a chance to
> > > > read
> > > > the extra bytes they might need.
> > > 
> > > I hesitate to add patches that extend image format probing.  For the
> > > past few years we have always recommended that image files should not be
> > > probed.
> > > 
> > > Image probing is prone to security issues because a malicious guest can
> > > modify a raw or vpc image by putting another image format header at
> > > sector 0.  The next time QEMU opens the image it will detect a different
> > > format.  One evil trick is to refer to a file on the host file system as
> > > the backing file, now you can read any file that the QEMU process has
> > > access to.
> 
> Yea, good point. The current state of probing is actually quite bad,
> just take a look at dmg_probe in block/dmg.c :-(
> 
> > > 
> > > Probing also complicates live migration.  The source host still has the
> > > image file open and may write to it.  The destination host shouldn't
> > > even read from the image file before handover to avoid file cache
> > > coherency issues.
> > > 
> > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
> > > (especially by adding more I/Os).
> 
> Even though, my series would have only added one extra I/O in the case
> of failing VPC images, I have to admit you are right.
> 
> > 
> > For 2.2, maybe we should limit probing to only certain operations (e.g.
> > qemu-img info) - or perhaps just remove the capability altogether, or
> > at least start phasing it out with a warning message that automatic
> > format detection is deprecated and may be unsafe.
> > 
> 
> Considering the fact that most open functions already check the magic
> numbers, and they do a lot better/safer job at it, we could just swap
> the probe functions with the open ones and just insert an fprintf
> when format is not specified doing what Jeff suggested.
> 
> Any objections to this?
> 
> (This would also solve the VPC-fixed-size bug, since vpc_open already
> checks the footer if the header is not found)
>

I was proposing actually going a bit further than this, and not
allowing automatic format detection at all, with an exception for
'qemu-img info'. In the interim, until that is in place and it is
removed, warn with a deprecation message.

Using the open function, while a bit more robust, still doesn't
prevent a raw image from masquerading as a file format - particularly
from a malevolent guest, as Stefan pointed out.  It just means the
malicious guest has to put forth minor effort in getting the metadata
correct.  I'm not sure it is worth the effort to overhaul the probe
system this way, rather than just warn about deprecation, and
eventually remote it altogether.

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Kevin Wolf]

Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
> On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
> > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
> > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> > > > > Fixed size VPC images do not have a footer, hence the current probe
> > > > > function will fail and QEMU will fall back to the raw_bsd driver, which
> > > > > is
> > > > > not the correct behaviour. The specification of the format says that
> > > > > fixed
> > > > > size images have a footer as the last 512 bytes of the file. The footer
> > > > > is
> > > > > exactly the same as the header would be in the case of dynamically
> > > > > growing
> > > > > images.
> > > > > 
> > > > > For this, we need to read the last 512 bytes of the image, however the
> > > > > current mechanics predominantly read the first 2048 bytes and pass that
> > > > > as a buffer to the probe functions. Solve this by passing the
> > > > > BlockDriverState to the probe functions, hence giving them a chance to
> > > > > read
> > > > > the extra bytes they might need.
> > > > 
> > > > I hesitate to add patches that extend image format probing.  For the
> > > > past few years we have always recommended that image files should not be
> > > > probed.
> > > > 
> > > > Image probing is prone to security issues because a malicious guest can
> > > > modify a raw or vpc image by putting another image format header at
> > > > sector 0.  The next time QEMU opens the image it will detect a different
> > > > format.  One evil trick is to refer to a file on the host file system as
> > > > the backing file, now you can read any file that the QEMU process has
> > > > access to.
> > 
> > Yea, good point. The current state of probing is actually quite bad,
> > just take a look at dmg_probe in block/dmg.c :-(
> > 
> > > > 
> > > > Probing also complicates live migration.  The source host still has the
> > > > image file open and may write to it.  The destination host shouldn't
> > > > even read from the image file before handover to avoid file cache
> > > > coherency issues.
> > > > 
> > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
> > > > (especially by adding more I/Os).
> > 
> > Even though, my series would have only added one extra I/O in the case
> > of failing VPC images, I have to admit you are right.
> > 
> > > 
> > > For 2.2, maybe we should limit probing to only certain operations (e.g.
> > > qemu-img info) - or perhaps just remove the capability altogether, or
> > > at least start phasing it out with a warning message that automatic
> > > format detection is deprecated and may be unsafe.
> > > 
> > 
> > Considering the fact that most open functions already check the magic
> > numbers, and they do a lot better/safer job at it, we could just swap
> > the probe functions with the open ones and just insert an fprintf
> > when format is not specified doing what Jeff suggested.
> > 
> > Any objections to this?
> > 
> > (This would also solve the VPC-fixed-size bug, since vpc_open already
> > checks the footer if the header is not found)
> >
> 
> I was proposing actually going a bit further than this, and not
> allowing automatic format detection at all, with an exception for
> 'qemu-img info'. In the interim, until that is in place and it is
> removed, warn with a deprecation message.

No, we can't do this. It would immediately destroy -hda and similar
convenience options and make the command line really hard to use even
for simple cases. I usually call qemu manually and I specify format
basically _never_, because it would like double the length of my command
line (okay, not quite, but my command lines are usually very short) and
I know what I'm doing and I'm running trusted guests.

Plus, there are probably many scripts out there that rely on it.

A more reasonable approach would be to just forbid probing raw and
raw-like formats like VHD fixed (the rest should be safe), but I think
the impact of this would still be too bad.

Kevin

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
>> On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
>> > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
>> > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
>> > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
>> > > > > Fixed size VPC images do not have a footer, hence the current probe
>> > > > > function will fail and QEMU will fall back to the raw_bsd driver, which
>> > > > > is
>> > > > > not the correct behaviour. The specification of the format says that
>> > > > > fixed
>> > > > > size images have a footer as the last 512 bytes of the file. The footer
>> > > > > is
>> > > > > exactly the same as the header would be in the case of dynamically
>> > > > > growing
>> > > > > images.
>> > > > > 
>> > > > > For this, we need to read the last 512 bytes of the image, however the
>> > > > > current mechanics predominantly read the first 2048 bytes and pass that
>> > > > > as a buffer to the probe functions. Solve this by passing the
>> > > > > BlockDriverState to the probe functions, hence giving them a chance to
>> > > > > read
>> > > > > the extra bytes they might need.
>> > > > 
>> > > > I hesitate to add patches that extend image format probing.  For the
>> > > > past few years we have always recommended that image files should not be
>> > > > probed.
>> > > > 
>> > > > Image probing is prone to security issues because a malicious guest can
>> > > > modify a raw or vpc image by putting another image format header at
>> > > > sector 0.  The next time QEMU opens the image it will detect a different
>> > > > format.  One evil trick is to refer to a file on the host file system as
>> > > > the backing file, now you can read any file that the QEMU process has
>> > > > access to.
>> > 
>> > Yea, good point. The current state of probing is actually quite bad,
>> > just take a look at dmg_probe in block/dmg.c :-(
>> > 
>> > > > 
>> > > > Probing also complicates live migration.  The source host still has the
>> > > > image file open and may write to it.  The destination host shouldn't
>> > > > even read from the image file before handover to avoid file cache
>> > > > coherency issues.
>> > > > 
>> > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
>> > > > (especially by adding more I/Os).
>> > 
>> > Even though, my series would have only added one extra I/O in the case
>> > of failing VPC images, I have to admit you are right.
>> > 
>> > > 
>> > > For 2.2, maybe we should limit probing to only certain operations (e.g.
>> > > qemu-img info) - or perhaps just remove the capability altogether, or
>> > > at least start phasing it out with a warning message that automatic
>> > > format detection is deprecated and may be unsafe.
>> > > 
>> > 
>> > Considering the fact that most open functions already check the magic
>> > numbers, and they do a lot better/safer job at it, we could just swap
>> > the probe functions with the open ones and just insert an fprintf
>> > when format is not specified doing what Jeff suggested.
>> > 
>> > Any objections to this?
>> > 
>> > (This would also solve the VPC-fixed-size bug, since vpc_open already
>> > checks the footer if the header is not found)
>> >
>> 
>> I was proposing actually going a bit further than this, and not
>> allowing automatic format detection at all, with an exception for
>> 'qemu-img info'. In the interim, until that is in place and it is
>> removed, warn with a deprecation message.
>
> No, we can't do this. It would immediately destroy -hda and similar
> convenience options and make the command line really hard to use even
> for simple cases. I usually call qemu manually and I specify format
> basically _never_, because it would like double the length of my command
> line (okay, not quite, but my command lines are usually very short) and
> I know what I'm doing and I'm running trusted guests.
>
> Plus, there are probably many scripts out there that rely on it.
>
> A more reasonable approach would be to just forbid probing raw and
> raw-like formats like VHD fixed (the rest should be safe), but I think
> the impact of this would still be too bad.

I think we're doing our users a disservice by sticking to the fatally
flawed probing.  "Broken by default" is just wrong, and "convenience" is
no excuse.

I believe we can retain 90% of the convenience without the flaws, by
defaulting format based on file meta-data only: name and struct stat.

This breaks down for things like QCOW2-formatted logical volumes.  But
those things are exactly *not* the things casual users need.

It also breaks down when users call their QCOW2 images foo.vmdk, but I'd
call that a feature :)

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Fri, Aug 15, 2014 at 12:55:19PM +0200, Kevin Wolf wrote:
> Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
> > On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
> > > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
> > > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> > > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> > > > > > Fixed size VPC images do not have a footer, hence the current probe
> > > > > > function will fail and QEMU will fall back to the raw_bsd driver, which
> > > > > > is
> > > > > > not the correct behaviour. The specification of the format says that
> > > > > > fixed
> > > > > > size images have a footer as the last 512 bytes of the file. The footer
> > > > > > is
> > > > > > exactly the same as the header would be in the case of dynamically
> > > > > > growing
> > > > > > images.
> > > > > > 
> > > > > > For this, we need to read the last 512 bytes of the image, however the
> > > > > > current mechanics predominantly read the first 2048 bytes and pass that
> > > > > > as a buffer to the probe functions. Solve this by passing the
> > > > > > BlockDriverState to the probe functions, hence giving them a chance to
> > > > > > read
> > > > > > the extra bytes they might need.
> > > > > 
> > > > > I hesitate to add patches that extend image format probing.  For the
> > > > > past few years we have always recommended that image files should not be
> > > > > probed.
> > > > > 
> > > > > Image probing is prone to security issues because a malicious guest can
> > > > > modify a raw or vpc image by putting another image format header at
> > > > > sector 0.  The next time QEMU opens the image it will detect a different
> > > > > format.  One evil trick is to refer to a file on the host file system as
> > > > > the backing file, now you can read any file that the QEMU process has
> > > > > access to.
> > > 
> > > Yea, good point. The current state of probing is actually quite bad,
> > > just take a look at dmg_probe in block/dmg.c :-(
> > > 
> > > > > 
> > > > > Probing also complicates live migration.  The source host still has the
> > > > > image file open and may write to it.  The destination host shouldn't
> > > > > even read from the image file before handover to avoid file cache
> > > > > coherency issues.
> > > > > 
> > > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
> > > > > (especially by adding more I/Os).
> > > 
> > > Even though, my series would have only added one extra I/O in the case
> > > of failing VPC images, I have to admit you are right.
> > > 
> > > > 
> > > > For 2.2, maybe we should limit probing to only certain operations (e.g.
> > > > qemu-img info) - or perhaps just remove the capability altogether, or
> > > > at least start phasing it out with a warning message that automatic
> > > > format detection is deprecated and may be unsafe.
> > > > 
> > > 
> > > Considering the fact that most open functions already check the magic
> > > numbers, and they do a lot better/safer job at it, we could just swap
> > > the probe functions with the open ones and just insert an fprintf
> > > when format is not specified doing what Jeff suggested.
> > > 
> > > Any objections to this?
> > > 
> > > (This would also solve the VPC-fixed-size bug, since vpc_open already
> > > checks the footer if the header is not found)
> > >
> > 
> > I was proposing actually going a bit further than this, and not
> > allowing automatic format detection at all, with an exception for
> > 'qemu-img info'. In the interim, until that is in place and it is
> > removed, warn with a deprecation message.
> 
> No, we can't do this. It would immediately destroy -hda and similar
> convenience options and make the command line really hard to use even
> for simple cases. 
>

You are right, we would need a way to specify the image format for the
convenience options (and remove a lot of the convenience).  Perhaps
that is a show-stopper right away.

The other big area of impact is image files that have metadata
specifying a backing file, but not the backing file format.  For
instance, this usage still probes when opening 'foo.qcow2':
    qemu-img create -f qcow2 foo.qcow2 1G
    qemu-img create -f qcow2 bar.qcow2 -b foo.qcow2

    qemu-kvm -drive file=bar.qcow2,format=qcow2


As an aside - I've heard us developers say multiple places that we
really recommend against image probing, but our command line options
and user documentation don't reflect that - at least not strongly.
(For qemu-img, the documentation even says image format is "guessed
automatically in most cases").

> I usually call qemu manually and I specify format
> basically _never_, because it would like double the length of my command
> line (okay, not quite, but my command lines are usually very short) and
> I know what I'm doing and I'm running trusted guests.
> 

Yes, I do the same, and it is pretty convenient.  This change would
be somewhat of a pita.

> Plus, there are probably many scripts out there that rely on it.
> 
> A more reasonable approach would be to just forbid probing raw and
> raw-like formats like VHD fixed (the rest should be safe), but I think
> the impact of this would still be too bad.
>

I made some patches to add in a deprecation message, and an additional
option so 'qemu-img info' could bypass that message.

Things immediately noticed:

* qemu-io didn't have a way to specify image format easily, so that
  needed to be added.
* qemu-img didn't have a way to specify image format for some options;
  that needed added as well.
* pretty much every qemu io test needed to be modified, to explicitly
  specify image format.  I'm about 25% through those.

And of course, convenience options like -hda spit out the deprecation
warning - which I think is probably a good thing.  Here is what I made
it say:

      fprintf(stderr, "Format autodetection is deprecated and may be "
              "removed in future versions.  Image format autodetection "
              "is not reliable; some image formats (e.g. raw) may "
              "masquerade as other image formats.  This could lead to "
              "system data loss or leaks.\n");
  

If we think doing this is a good thing, I'll continue modifying the
qemu-iotests.  Otherwise, I'll drop it.

Allowing qemu-io to specify image format easily is probably a good
idea either way.

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Fri, Aug 15, 2014 at 01:21:19PM +0200, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
> >> On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
> >> > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
> >> > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> >> > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> >> > > > > Fixed size VPC images do not have a footer, hence the current probe
> >> > > > > function will fail and QEMU will fall back to the raw_bsd driver, which
> >> > > > > is
> >> > > > > not the correct behaviour. The specification of the format says that
> >> > > > > fixed
> >> > > > > size images have a footer as the last 512 bytes of the file. The footer
> >> > > > > is
> >> > > > > exactly the same as the header would be in the case of dynamically
> >> > > > > growing
> >> > > > > images.
> >> > > > > 
> >> > > > > For this, we need to read the last 512 bytes of the image, however the
> >> > > > > current mechanics predominantly read the first 2048 bytes and pass that
> >> > > > > as a buffer to the probe functions. Solve this by passing the
> >> > > > > BlockDriverState to the probe functions, hence giving them a chance to
> >> > > > > read
> >> > > > > the extra bytes they might need.
> >> > > > 
> >> > > > I hesitate to add patches that extend image format probing.  For the
> >> > > > past few years we have always recommended that image files should not be
> >> > > > probed.
> >> > > > 
> >> > > > Image probing is prone to security issues because a malicious guest can
> >> > > > modify a raw or vpc image by putting another image format header at
> >> > > > sector 0.  The next time QEMU opens the image it will detect a different
> >> > > > format.  One evil trick is to refer to a file on the host file system as
> >> > > > the backing file, now you can read any file that the QEMU process has
> >> > > > access to.
> >> > 
> >> > Yea, good point. The current state of probing is actually quite bad,
> >> > just take a look at dmg_probe in block/dmg.c :-(
> >> > 
> >> > > > 
> >> > > > Probing also complicates live migration.  The source host still has the
> >> > > > image file open and may write to it.  The destination host shouldn't
> >> > > > even read from the image file before handover to avoid file cache
> >> > > > coherency issues.
> >> > > > 
> >> > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
> >> > > > (especially by adding more I/Os).
> >> > 
> >> > Even though, my series would have only added one extra I/O in the case
> >> > of failing VPC images, I have to admit you are right.
> >> > 
> >> > > 
> >> > > For 2.2, maybe we should limit probing to only certain operations (e.g.
> >> > > qemu-img info) - or perhaps just remove the capability altogether, or
> >> > > at least start phasing it out with a warning message that automatic
> >> > > format detection is deprecated and may be unsafe.
> >> > > 
> >> > 
> >> > Considering the fact that most open functions already check the magic
> >> > numbers, and they do a lot better/safer job at it, we could just swap
> >> > the probe functions with the open ones and just insert an fprintf
> >> > when format is not specified doing what Jeff suggested.
> >> > 
> >> > Any objections to this?
> >> > 
> >> > (This would also solve the VPC-fixed-size bug, since vpc_open already
> >> > checks the footer if the header is not found)
> >> >
> >> 
> >> I was proposing actually going a bit further than this, and not
> >> allowing automatic format detection at all, with an exception for
> >> 'qemu-img info'. In the interim, until that is in place and it is
> >> removed, warn with a deprecation message.
> >
> > No, we can't do this. It would immediately destroy -hda and similar
> > convenience options and make the command line really hard to use even
> > for simple cases. I usually call qemu manually and I specify format
> > basically _never_, because it would like double the length of my command
> > line (okay, not quite, but my command lines are usually very short) and
> > I know what I'm doing and I'm running trusted guests.
> >
> > Plus, there are probably many scripts out there that rely on it.
> >
> > A more reasonable approach would be to just forbid probing raw and
> > raw-like formats like VHD fixed (the rest should be safe), but I think
> > the impact of this would still be too bad.
> 
> I think we're doing our users a disservice by sticking to the fatally
> flawed probing.  "Broken by default" is just wrong, and "convenience" is
> no excuse.
> 
> I believe we can retain 90% of the convenience without the flaws, by
> defaulting format based on file meta-data only: name and struct stat.

I worry that will subtly alter current behavior in bad ways.  For
instance, take this image chain:

    qemu-img create -f qcow2 foo.img 1G
    qemu-img create -f qcow2 -b foo.img bar.img 1G

    qemu-kvm -drive file=bar.img,format=qcow2


If I understand correctly what you are proposing, that means that
qemu-kvm would detect 'foo.img' as raw, while current behavior is to
detect it as 'qcow2'.

Although if we do that in conjunction with what Kevin proposed (forbid
probing on raw), it would behave 'properly', and bail out before doing
something bad.  That could be OK.

> 
> This breaks down for things like QCOW2-formatted logical volumes.  But
> those things are exactly *not* the things casual users need.
> 
> It also breaks down when users call their QCOW2 images foo.vmdk, but I'd
> call that a feature :)

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Fri, Aug 15, 2014 at 01:21:19PM +0200, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
>> >> On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
>> >> > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
>> >> > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
>> >> > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
>> >> > > > > Fixed size VPC images do not have a footer, hence the current probe
>> >> > > > > function will fail and QEMU will fall back to the raw_bsd
>> >> > > > > driver, which
>> >> > > > > is
>> >> > > > > not the correct behaviour. The specification of the format says that
>> >> > > > > fixed
>> >> > > > > size images have a footer as the last 512 bytes of the
>> >> > > > > file. The footer
>> >> > > > > is
>> >> > > > > exactly the same as the header would be in the case of dynamically
>> >> > > > > growing
>> >> > > > > images.
>> >> > > > > 
>> >> > > > > For this, we need to read the last 512 bytes of the
>> >> > > > > image, however the
>> >> > > > > current mechanics predominantly read the first 2048 bytes
>> >> > > > > and pass that
>> >> > > > > as a buffer to the probe functions. Solve this by passing the
>> >> > > > > BlockDriverState to the probe functions, hence giving
>> >> > > > > them a chance to
>> >> > > > > read
>> >> > > > > the extra bytes they might need.
>> >> > > > 
>> >> > > > I hesitate to add patches that extend image format probing.  For the
>> >> > > > past few years we have always recommended that image files
>> >> > > > should not be
>> >> > > > probed.
>> >> > > > 
>> >> > > > Image probing is prone to security issues because a
>> >> > > > malicious guest can
>> >> > > > modify a raw or vpc image by putting another image format header at
>> >> > > > sector 0.  The next time QEMU opens the image it will
>> >> > > > detect a different
>> >> > > > format.  One evil trick is to refer to a file on the host
>> >> > > > file system as
>> >> > > > the backing file, now you can read any file that the QEMU process has
>> >> > > > access to.
>> >> > 
>> >> > Yea, good point. The current state of probing is actually quite bad,
>> >> > just take a look at dmg_probe in block/dmg.c :-(
>> >> > 
>> >> > > > 
>> >> > > > Probing also complicates live migration.  The source host
>> >> > > > still has the
>> >> > > > image file open and may write to it.  The destination host shouldn't
>> >> > > > even read from the image file before handover to avoid file cache
>> >> > > > coherency issues.
>> >> > > > 
>> >> > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
>> >> > > > (especially by adding more I/Os).
>> >> > 
>> >> > Even though, my series would have only added one extra I/O in the case
>> >> > of failing VPC images, I have to admit you are right.
>> >> > 
>> >> > > 
>> >> > > For 2.2, maybe we should limit probing to only certain operations (e.g.
>> >> > > qemu-img info) - or perhaps just remove the capability altogether, or
>> >> > > at least start phasing it out with a warning message that automatic
>> >> > > format detection is deprecated and may be unsafe.
>> >> > > 
>> >> > 
>> >> > Considering the fact that most open functions already check the magic
>> >> > numbers, and they do a lot better/safer job at it, we could just swap
>> >> > the probe functions with the open ones and just insert an fprintf
>> >> > when format is not specified doing what Jeff suggested.
>> >> > 
>> >> > Any objections to this?
>> >> > 
>> >> > (This would also solve the VPC-fixed-size bug, since vpc_open already
>> >> > checks the footer if the header is not found)
>> >> >
>> >> 
>> >> I was proposing actually going a bit further than this, and not
>> >> allowing automatic format detection at all, with an exception for
>> >> 'qemu-img info'. In the interim, until that is in place and it is
>> >> removed, warn with a deprecation message.
>> >
>> > No, we can't do this. It would immediately destroy -hda and similar
>> > convenience options and make the command line really hard to use even
>> > for simple cases. I usually call qemu manually and I specify format
>> > basically _never_, because it would like double the length of my command
>> > line (okay, not quite, but my command lines are usually very short) and
>> > I know what I'm doing and I'm running trusted guests.
>> >
>> > Plus, there are probably many scripts out there that rely on it.
>> >
>> > A more reasonable approach would be to just forbid probing raw and
>> > raw-like formats like VHD fixed (the rest should be safe), but I think
>> > the impact of this would still be too bad.
>> 
>> I think we're doing our users a disservice by sticking to the fatally
>> flawed probing.  "Broken by default" is just wrong, and "convenience" is
>> no excuse.
>> 
>> I believe we can retain 90% of the convenience without the flaws, by
>> defaulting format based on file meta-data only: name and struct stat.
>
> I worry that will subtly alter current behavior in bad ways.  For

Oh, yes, it does alter current behavior.

> instance, take this image chain:
>
>     qemu-img create -f qcow2 foo.img 1G
>     qemu-img create -f qcow2 -b foo.img bar.img 1G
>
>     qemu-kvm -drive file=bar.img,format=qcow2
>
>
> If I understand correctly what you are proposing, that means that
> qemu-kvm would detect 'foo.img' as raw, while current behavior is to
> detect it as 'qcow2'.

Correct.  Educate people to call it foo.qcow2 already, or to add
format=raw.

> Although if we do that in conjunction with what Kevin proposed (forbid
> probing on raw), it would behave 'properly', and bail out before doing
> something bad.  That could be OK.

The obvious way forward is to start warning folks when we pick a format
based on image contents, unless it matches the pick based on image name
and stat.  Do that for a year or three, then switch over.

If we'd done that back when we discovered the flaw (CVE-2008-2004), our
users would've been safe by default for several years now.

[...]

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Eric Blake]

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

On 08/15/2014 06:28 AM, Jeff Cody wrote:

> I worry that will subtly alter current behavior in bad ways.  For
> instance, take this image chain:
> 
>     qemu-img create -f qcow2 foo.img 1G
>     qemu-img create -f qcow2 -b foo.img bar.img 1G
> 
>     qemu-kvm -drive file=bar.img,format=qcow2
> 
> 
> If I understand correctly what you are proposing, that means that
> qemu-kvm would detect 'foo.img' as raw, while current behavior is to
> detect it as 'qcow2'.
> 

Libvirt ALREADY defaults to detecting foo.img as raw, and refuses to
grant SELinux permissions for qemu to read bar.img, which causes qemu to
fail to start due to missing permissions.  All because probing is deemed
too dangerous (a probe that results in an answer of "raw" is
trustworthy, a probe that results in any other answer is suspect if the
file has any remote chance of having once been raw).

> Although if we do that in conjunction with what Kevin proposed (forbid
> probing on raw), it would behave 'properly', and bail out before doing
> something bad.  That could be OK.

The problem is that you can't forbid probing on raw without forbidding
probing almost everywhere.  Again, an answer of "raw" is trustworthy, it
is ALL OTHER answers that are suspect.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Eric Blake]

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

On 08/15/2014 06:14 AM, Jeff Cody wrote:

> 
> And of course, convenience options like -hda spit out the deprecation
> warning - which I think is probably a good thing.  Here is what I made
> it say:
> 
>       fprintf(stderr, "Format autodetection is deprecated and may be "
>               "removed in future versions.  Image format autodetection "
>               "is not reliable; some image formats (e.g. raw) may "
>               "masquerade as other image formats.  This could lead to "
>               "system data loss or leaks.\n");
>   
> 
> If we think doing this is a good thing, I'll continue modifying the
> qemu-iotests.  Otherwise, I'll drop it.
> 

I'm in favor of it. The original CVE against qemu (CVE-2008-2004) has
resulted in multiple libvirt CVEs over the years in dealing with
fallout; most recently, there was debate just this year on whether a
libvirt bug dealing with incorrect probing during drive-mirror
situations counted as a CVE (the determination was that because
libvirt's default is to prohibit probing, it did not; a user that
intentionally flips libvirt's configuration to again allow probing has
self-inflicted the vulnerability that I had uncovered).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Fri, Aug 15, 2014 at 07:13:07AM -0600, Eric Blake wrote:
> On 08/15/2014 06:28 AM, Jeff Cody wrote:
> 
> > I worry that will subtly alter current behavior in bad ways.  For
> > instance, take this image chain:
> > 
> >     qemu-img create -f qcow2 foo.img 1G
> >     qemu-img create -f qcow2 -b foo.img bar.img 1G
> > 
> >     qemu-kvm -drive file=bar.img,format=qcow2
> > 
> > 
> > If I understand correctly what you are proposing, that means that
> > qemu-kvm would detect 'foo.img' as raw, while current behavior is to
> > detect it as 'qcow2'.
> > 
> 
> Libvirt ALREADY defaults to detecting foo.img as raw, and refuses to
> grant SELinux permissions for qemu to read bar.img, which causes qemu to
> fail to start due to missing permissions.  All because probing is deemed
> too dangerous (a probe that results in an answer of "raw" is
> trustworthy, a probe that results in any other answer is suspect if the
> file has any remote chance of having once been raw).
> 
> > Although if we do that in conjunction with what Kevin proposed (forbid
> > probing on raw), it would behave 'properly', and bail out before doing
> > something bad.  That could be OK.
> 
> The problem is that you can't forbid probing on raw without forbidding
> probing almost everywhere.  Again, an answer of "raw" is trustworthy, it
> is ALL OTHER answers that are suspect.
> 
> 

I agree that raw is trustworthy (as in, the safest default).  My point
is that I think that silently changing behavior on existing chains
(not everyone uses libvirt and selinux rules) would be bad for
existing users.  I think it best to explicitly warn, and then
deprecate.

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Kevin Wolf]

Am 15.08.2014 um 14:14 hat Jeff Cody geschrieben:
> On Fri, Aug 15, 2014 at 12:55:19PM +0200, Kevin Wolf wrote:
> > Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
> > > On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
> > > > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
> > > > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> > > > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> > > > > > > Fixed size VPC images do not have a footer, hence the current probe
> > > > > > > function will fail and QEMU will fall back to the raw_bsd driver, which
> > > > > > > is
> > > > > > > not the correct behaviour. The specification of the format says that
> > > > > > > fixed
> > > > > > > size images have a footer as the last 512 bytes of the file. The footer
> > > > > > > is
> > > > > > > exactly the same as the header would be in the case of dynamically
> > > > > > > growing
> > > > > > > images.
> > > > > > > 
> > > > > > > For this, we need to read the last 512 bytes of the image, however the
> > > > > > > current mechanics predominantly read the first 2048 bytes and pass that
> > > > > > > as a buffer to the probe functions. Solve this by passing the
> > > > > > > BlockDriverState to the probe functions, hence giving them a chance to
> > > > > > > read
> > > > > > > the extra bytes they might need.
> > > > > > 
> > > > > > I hesitate to add patches that extend image format probing.  For the
> > > > > > past few years we have always recommended that image files should not be
> > > > > > probed.
> > > > > > 
> > > > > > Image probing is prone to security issues because a malicious guest can
> > > > > > modify a raw or vpc image by putting another image format header at
> > > > > > sector 0.  The next time QEMU opens the image it will detect a different
> > > > > > format.  One evil trick is to refer to a file on the host file system as
> > > > > > the backing file, now you can read any file that the QEMU process has
> > > > > > access to.
> > > > 
> > > > Yea, good point. The current state of probing is actually quite bad,
> > > > just take a look at dmg_probe in block/dmg.c :-(
> > > > 
> > > > > > 
> > > > > > Probing also complicates live migration.  The source host still has the
> > > > > > image file open and may write to it.  The destination host shouldn't
> > > > > > even read from the image file before handover to avoid file cache
> > > > > > coherency issues.
> > > > > > 
> > > > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
> > > > > > (especially by adding more I/Os).
> > > > 
> > > > Even though, my series would have only added one extra I/O in the case
> > > > of failing VPC images, I have to admit you are right.
> > > > 
> > > > > 
> > > > > For 2.2, maybe we should limit probing to only certain operations (e.g.
> > > > > qemu-img info) - or perhaps just remove the capability altogether, or
> > > > > at least start phasing it out with a warning message that automatic
> > > > > format detection is deprecated and may be unsafe.
> > > > > 
> > > > 
> > > > Considering the fact that most open functions already check the magic
> > > > numbers, and they do a lot better/safer job at it, we could just swap
> > > > the probe functions with the open ones and just insert an fprintf
> > > > when format is not specified doing what Jeff suggested.
> > > > 
> > > > Any objections to this?
> > > > 
> > > > (This would also solve the VPC-fixed-size bug, since vpc_open already
> > > > checks the footer if the header is not found)
> > > >
> > > 
> > > I was proposing actually going a bit further than this, and not
> > > allowing automatic format detection at all, with an exception for
> > > 'qemu-img info'. In the interim, until that is in place and it is
> > > removed, warn with a deprecation message.
> > 
> > No, we can't do this. It would immediately destroy -hda and similar
> > convenience options and make the command line really hard to use even
> > for simple cases. 
> >
> 
> You are right, we would need a way to specify the image format for the
> convenience options (and remove a lot of the convenience).  Perhaps
> that is a show-stopper right away.

I think it's very close to a show-stopper anyway.

But it wouldn't hurt to allow something like -cdrom test.iso,format=raw
(or any other -drive options), that already saves some typing compared
to -drive.

> The other big area of impact is image files that have metadata
> specifying a backing file, but not the backing file format.  For
> instance, this usage still probes when opening 'foo.qcow2':
>     qemu-img create -f qcow2 foo.qcow2 1G
>     qemu-img create -f qcow2 bar.qcow2 -b foo.qcow2
> 
>     qemu-kvm -drive file=bar.qcow2,format=qcow2

Ouch, you're right. If the above wasn't a show-stopper, this one is.
Breaking all images with backing files so that you need to fix up whole
backing file chains with rebase -u is not an option.

> As an aside - I've heard us developers say multiple places that we
> really recommend against image probing, but our command line options
> and user documentation don't reflect that - at least not strongly.
> (For qemu-img, the documentation even says image format is "guessed
> automatically in most cases").
> 
> > I usually call qemu manually and I specify format
> > basically _never_, because it would like double the length of my command
> > line (okay, not quite, but my command lines are usually very short) and
> > I know what I'm doing and I'm running trusted guests.
> > 
> 
> Yes, I do the same, and it is pretty convenient.  This change would
> be somewhat of a pita.
> 
> > Plus, there are probably many scripts out there that rely on it.
> > 
> > A more reasonable approach would be to just forbid probing raw and
> > raw-like formats like VHD fixed (the rest should be safe), but I think
> > the impact of this would still be too bad.
> >
> 
> I made some patches to add in a deprecation message, and an additional
> option so 'qemu-img info' could bypass that message.
> 
> Things immediately noticed:
> 
> * qemu-io didn't have a way to specify image format easily, so that
>   needed to be added.
> * qemu-img didn't have a way to specify image format for some options;
>   that needed added as well.

No matter if we actually deprecate anything or not, these changes are
definitely valuable.

> * pretty much every qemu io test needed to be modified, to explicitly
>   specify image format.  I'm about 25% through those.
> 
> And of course, convenience options like -hda spit out the deprecation
> warning - which I think is probably a good thing.  Here is what I made
> it say:
> 
>       fprintf(stderr, "Format autodetection is deprecated and may be "
>               "removed in future versions.  Image format autodetection "
>               "is not reliable; some image formats (e.g. raw) may "
>               "masquerade as other image formats.  This could lead to "
>               "system data loss or leaks.\n");
>   
> 
> If we think doing this is a good thing, I'll continue modifying the
> qemu-iotests.  Otherwise, I'll drop it.

I don't think this radical way works.

We can choose Markus's suggestion of using the file name to guess the
format. I don't really like it much, but it seems like a fair compromise
that doesn't hurt usability as much.

If we don't want this, we can approach the problem from a different
angle: The problem is not probing per se, but that images probed as raw
can be written to by guests in a way that the next time they are probed
as something else.

What if we let the raw driver know that it was probed and then it
enables a check that returns -EIO for any write on the first 2k if that
write would make the image look like a different format?

Kevin

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Fri, Aug 15, 2014 at 03:37:56PM +0200, Kevin Wolf wrote:
> Am 15.08.2014 um 14:14 hat Jeff Cody geschrieben:
> > On Fri, Aug 15, 2014 at 12:55:19PM +0200, Kevin Wolf wrote:
> > > Am 14.08.2014 um 16:57 hat Jeff Cody geschrieben:
> > > > On Thu, Aug 14, 2014 at 10:42:27AM -0400, Levente Kurusa wrote:
> > > > > On Tuesday, 12 August, 2014 3:35:42 PM, Jeff Cody wrote:
> > > > > > On Tue, Aug 12, 2014 at 02:20:34PM +0100, Stefan Hajnoczi wrote:
> > > > > > > On Fri, Aug 01, 2014 at 03:39:58PM +0200, Levente Kurusa wrote:
> > > > > > > > Fixed size VPC images do not have a footer, hence the current probe
> > > > > > > > function will fail and QEMU will fall back to the raw_bsd driver, which
> > > > > > > > is
> > > > > > > > not the correct behaviour. The specification of the format says that
> > > > > > > > fixed
> > > > > > > > size images have a footer as the last 512 bytes of the file. The footer
> > > > > > > > is
> > > > > > > > exactly the same as the header would be in the case of dynamically
> > > > > > > > growing
> > > > > > > > images.
> > > > > > > > 
> > > > > > > > For this, we need to read the last 512 bytes of the image, however the
> > > > > > > > current mechanics predominantly read the first 2048 bytes and pass that
> > > > > > > > as a buffer to the probe functions. Solve this by passing the
> > > > > > > > BlockDriverState to the probe functions, hence giving them a chance to
> > > > > > > > read
> > > > > > > > the extra bytes they might need.
> > > > > > > 
> > > > > > > I hesitate to add patches that extend image format probing.  For the
> > > > > > > past few years we have always recommended that image files should not be
> > > > > > > probed.
> > > > > > > 
> > > > > > > Image probing is prone to security issues because a malicious guest can
> > > > > > > modify a raw or vpc image by putting another image format header at
> > > > > > > sector 0.  The next time QEMU opens the image it will detect a different
> > > > > > > format.  One evil trick is to refer to a file on the host file system as
> > > > > > > the backing file, now you can read any file that the QEMU process has
> > > > > > > access to.
> > > > > 
> > > > > Yea, good point. The current state of probing is actually quite bad,
> > > > > just take a look at dmg_probe in block/dmg.c :-(
> > > > > 
> > > > > > > 
> > > > > > > Probing also complicates live migration.  The source host still has the
> > > > > > > image file open and may write to it.  The destination host shouldn't
> > > > > > > even read from the image file before handover to avoid file cache
> > > > > > > coherency issues.
> > > > > > > 
> > > > > > > Probing is broken.  It shouldn't be used.  We shouldn't extend it
> > > > > > > (especially by adding more I/Os).
> > > > > 
> > > > > Even though, my series would have only added one extra I/O in the case
> > > > > of failing VPC images, I have to admit you are right.
> > > > > 
> > > > > > 
> > > > > > For 2.2, maybe we should limit probing to only certain operations (e.g.
> > > > > > qemu-img info) - or perhaps just remove the capability altogether, or
> > > > > > at least start phasing it out with a warning message that automatic
> > > > > > format detection is deprecated and may be unsafe.
> > > > > > 
> > > > > 
> > > > > Considering the fact that most open functions already check the magic
> > > > > numbers, and they do a lot better/safer job at it, we could just swap
> > > > > the probe functions with the open ones and just insert an fprintf
> > > > > when format is not specified doing what Jeff suggested.
> > > > > 
> > > > > Any objections to this?
> > > > > 
> > > > > (This would also solve the VPC-fixed-size bug, since vpc_open already
> > > > > checks the footer if the header is not found)
> > > > >
> > > > 
> > > > I was proposing actually going a bit further than this, and not
> > > > allowing automatic format detection at all, with an exception for
> > > > 'qemu-img info'. In the interim, until that is in place and it is
> > > > removed, warn with a deprecation message.
> > > 
> > > No, we can't do this. It would immediately destroy -hda and similar
> > > convenience options and make the command line really hard to use even
> > > for simple cases. 
> > >
> > 
> > You are right, we would need a way to specify the image format for the
> > convenience options (and remove a lot of the convenience).  Perhaps
> > that is a show-stopper right away.
> 
> I think it's very close to a show-stopper anyway.
> 
> But it wouldn't hurt to allow something like -cdrom test.iso,format=raw
> (or any other -drive options), that already saves some typing compared
> to -drive.
> 
> > The other big area of impact is image files that have metadata
> > specifying a backing file, but not the backing file format.  For
> > instance, this usage still probes when opening 'foo.qcow2':
> >     qemu-img create -f qcow2 foo.qcow2 1G
> >     qemu-img create -f qcow2 bar.qcow2 -b foo.qcow2
> > 
> >     qemu-kvm -drive file=bar.qcow2,format=qcow2
> 
> Ouch, you're right. If the above wasn't a show-stopper, this one is.
> Breaking all images with backing files so that you need to fix up whole
> backing file chains with rebase -u is not an option.
> 
> > As an aside - I've heard us developers say multiple places that we
> > really recommend against image probing, but our command line options
> > and user documentation don't reflect that - at least not strongly.
> > (For qemu-img, the documentation even says image format is "guessed
> > automatically in most cases").
> > 
> > > I usually call qemu manually and I specify format
> > > basically _never_, because it would like double the length of my command
> > > line (okay, not quite, but my command lines are usually very short) and
> > > I know what I'm doing and I'm running trusted guests.
> > > 
> > 
> > Yes, I do the same, and it is pretty convenient.  This change would
> > be somewhat of a pita.
> > 
> > > Plus, there are probably many scripts out there that rely on it.
> > > 
> > > A more reasonable approach would be to just forbid probing raw and
> > > raw-like formats like VHD fixed (the rest should be safe), but I think
> > > the impact of this would still be too bad.
> > >
> > 
> > I made some patches to add in a deprecation message, and an additional
> > option so 'qemu-img info' could bypass that message.
> > 
> > Things immediately noticed:
> > 
> > * qemu-io didn't have a way to specify image format easily, so that
> >   needed to be added.
> > * qemu-img didn't have a way to specify image format for some options;
> >   that needed added as well.
> 
> No matter if we actually deprecate anything or not, these changes are
> definitely valuable.
> 
> > * pretty much every qemu io test needed to be modified, to explicitly
> >   specify image format.  I'm about 25% through those.
> > 
> > And of course, convenience options like -hda spit out the deprecation
> > warning - which I think is probably a good thing.  Here is what I made
> > it say:
> > 
> >       fprintf(stderr, "Format autodetection is deprecated and may be "
> >               "removed in future versions.  Image format autodetection "
> >               "is not reliable; some image formats (e.g. raw) may "
> >               "masquerade as other image formats.  This could lead to "
> >               "system data loss or leaks.\n");
> >   
> > 
> > If we think doing this is a good thing, I'll continue modifying the
> > qemu-iotests.  Otherwise, I'll drop it.
> 
> I don't think this radical way works.
> 
> We can choose Markus's suggestion of using the file name to guess the
> format. I don't really like it much, but it seems like a fair compromise
> that doesn't hurt usability as much.
> 

Imagine the chain and cmdline described above, except someone created
images not with .qcow2 extension, but with .img extension.

If we go by file name, this happens when the user upgrades QEMU -

Old behavior:

  | foo.img | <--- | bar.img |
  | (qcow2) |      | (qcow2) |


New behavior:

  | foo.img | <--- | bar.img |
  | (raw)   |      | (qcow2) |

Which means the guest will almost certainly have data corruption.


> If we don't want this, we can approach the problem from a different
> angle: The problem is not probing per se, but that images probed as raw
> can be written to by guests in a way that the next time they are probed
> as something else.
> 
> What if we let the raw driver know that it was probed and then it
> enables a check that returns -EIO for any write on the first 2k if that
> write would make the image look like a different format?
>

I fear that would have even more ramifications, and lead to all sorts
of extra bugs and support (I can already see the block-commit bugs
now...).  And couldn't a guest still create a tricky
raw file, when format=raw was explicitly specified?  It would just
have to wait until some time that it was opened by probing.

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Eric Blake]

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

On 08/15/2014 07:37 AM, Kevin Wolf wrote:

> We can choose Markus's suggestion of using the file name to guess the
> format. I don't really like it much, but it seems like a fair compromise
> that doesn't hurt usability as much.

In other words, if a user gives a file a "known suffix", then it is
their own fault if they made that file raw and the guest then happened
to convert the file to the format matching the suffix?  Or would this
start giving warnings if the known suffix doesn't match the probed contents?

> 
> If we don't want this, we can approach the problem from a different
> angle: The problem is not probing per se, but that images probed as raw
> can be written to by guests in a way that the next time they are probed
> as something else.
> 
> What if we let the raw driver know that it was probed and then it
> enables a check that returns -EIO for any write on the first 2k if that
> write would make the image look like a different format?

Not entirely future-proof - as we add support for more formats over
time, something that passes today could fail in the future.  Worse, a
guest could exploit an older qemu to write a header that a newer qemu
would reject.  But it does sound like an interesting approach
(preventing the guest from doing something risky).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Fri, Aug 15, 2014 at 08:00:19AM -0600, Eric Blake wrote:
> On 08/15/2014 07:37 AM, Kevin Wolf wrote:
> 
> > We can choose Markus's suggestion of using the file name to guess the
> > format. I don't really like it much, but it seems like a fair compromise
> > that doesn't hurt usability as much.
> 
> In other words, if a user gives a file a "known suffix", then it is
> their own fault if they made that file raw and the guest then happened
> to convert the file to the format matching the suffix?  Or would this
> start giving warnings if the known suffix doesn't match the probed contents?
>

(Eric, I should have cc'ed you on my last email, sorry)

Image this scenario:

    existing chain created a while ago, via:

        qemu-img create -f qcow2 foo.img 1G
        qemu-img create -f qcow2 -b foo.img bar.img 1G


User launches qemu by this commandline:

    qemu-kvm -drive file=bar.img,format=qcow2


Old behavior:

  | foo.img | <--- | bar.img |
  | (qcow2) |      | (qcow2) |

New behavior:

  | foo.img | <--- | bar.img |
  | (raw)   |      | (qcow2) |


So I think we want to make sure that we don't just fall back to raw
for unknown filename extensions.


> > 
> > If we don't want this, we can approach the problem from a different
> > angle: The problem is not probing per se, but that images probed as raw
> > can be written to by guests in a way that the next time they are probed
> > as something else.
> > 
> > What if we let the raw driver know that it was probed and then it
> > enables a check that returns -EIO for any write on the first 2k if that
> > write would make the image look like a different format?
> 
> Not entirely future-proof - as we add support for more formats over
> time, something that passes today could fail in the future.  Worse, a
> guest could exploit an older qemu to write a header that a newer qemu
> would reject.  But it does sound like an interesting approach
> (preventing the guest from doing something risky).
> 

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Eric Blake]

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

On 08/15/2014 08:10 AM, Jeff Cody wrote:
> On Fri, Aug 15, 2014 at 08:00:19AM -0600, Eric Blake wrote:
>> On 08/15/2014 07:37 AM, Kevin Wolf wrote:
>>
>>> We can choose Markus's suggestion of using the file name to guess the
>>> format. I don't really like it much, but it seems like a fair compromise
>>> that doesn't hurt usability as much.
>>
>> In other words, if a user gives a file a "known suffix", then it is
>> their own fault if they made that file raw and the guest then happened
>> to convert the file to the format matching the suffix?  Or would this
>> start giving warnings if the known suffix doesn't match the probed contents?
>>
> 
> (Eric, I should have cc'ed you on my last email, sorry)
> 
> Image this scenario:
> 
>     existing chain created a while ago, via:
> 
>         qemu-img create -f qcow2 foo.img 1G
>         qemu-img create -f qcow2 -b foo.img bar.img 1G
> 

Libvirt refuses to create files without backing format encoded; but is
still stuck dealing with files created externally (such as qemu-img
create) where it was forgotten.

> 
> User launches qemu by this commandline:
> 
>     qemu-kvm -drive file=bar.img,format=qcow2
> 
> 
> Old behavior:
> 
>   | foo.img | <--- | bar.img |
>   | (qcow2) |      | (qcow2) |
> 
> New behavior:
> 
>   | foo.img | <--- | bar.img |
>   | (raw)   |      | (qcow2) |

Libvirt already assumed raw for any image where encoding was not
specified, so this scenario is already a risky one for the user.

> 
> 
> So I think we want to make sure that we don't just fall back to raw
> for unknown filename extensions.

Remember, if a probe determines a file is raw, it is safe to use as raw.
 Falling back to raw is IDEAL.

What _might_ be nice is teaching qemu-img to auto-encode the backing
format during 'qemu-img create' (it can be assumed that _at creation
time_, the backing file hasn't had a chance to be corrupted by the guest
yet) rather than the current behavior of only encoding an explicit
format given by the user.  Furthermore, an explicit encoding from the
user should ALWAYS be trusted, and the user can override any place where
automatic heuristics would have guessed differently (whether those
heuristics were based on probing, file extension, or otherwise).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Kevin Wolf]

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

Am 15.08.2014 um 16:00 hat Eric Blake geschrieben:
> On 08/15/2014 07:37 AM, Kevin Wolf wrote:
> 
> > We can choose Markus's suggestion of using the file name to guess the
> > format. I don't really like it much, but it seems like a fair compromise
> > that doesn't hurt usability as much.
> 
> In other words, if a user gives a file a "known suffix", then it is
> their own fault if they made that file raw and the guest then happened
> to convert the file to the format matching the suffix?  Or would this
> start giving warnings if the known suffix doesn't match the probed contents?

You mean if the suffix doesn't match the explicit format=... option?
Because after the malicious guest has done its work, it would match
again.

> > If we don't want this, we can approach the problem from a different
> > angle: The problem is not probing per se, but that images probed as raw
> > can be written to by guests in a way that the next time they are probed
> > as something else.
> > 
> > What if we let the raw driver know that it was probed and then it
> > enables a check that returns -EIO for any write on the first 2k if that
> > write would make the image look like a different format?
> 
> Not entirely future-proof - as we add support for more formats over
> time, something that passes today could fail in the future.  Worse, a
> guest could exploit an older qemu to write a header that a newer qemu
> would reject.  But it does sound like an interesting approach
> (preventing the guest from doing something risky).

With qemu updates (or different configure options), it could happen that
the behaviour changes, yes. But if a malicious guest has to figure out
how to write a header today that will allow it to read some file on the
host after the user replaces the qemu binary, that makes it probably
hard to exploit anything in practice.

And seriously, if I wanted to use the backing file to read something on
the host, I wouldn't mess with the guest OS trying to rewrite its raw
image into qcow2 while not destroying itself (has anyone tried to write
a proof of concept for this? It doesn't look trivial if you have e.g. a
normal Linux installation that is getting hacked), but just offer the
user some (real) qcow2 for download that already has the desired backing
file path in it...

Kevin

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

Re: [Qemu-devel] [PATCH 0/3] vpc: support probing of fixed size images

[Jeff Cody]

On Fri, Aug 15, 2014 at 08:22:12AM -0600, Eric Blake wrote:
> On 08/15/2014 08:10 AM, Jeff Cody wrote:
> > On Fri, Aug 15, 2014 at 08:00:19AM -0600, Eric Blake wrote:
> >> On 08/15/2014 07:37 AM, Kevin Wolf wrote:
> >>
> >>> We can choose Markus's suggestion of using the file name to guess the
> >>> format. I don't really like it much, but it seems like a fair compromise
> >>> that doesn't hurt usability as much.
> >>
> >> In other words, if a user gives a file a "known suffix", then it is
> >> their own fault if they made that file raw and the guest then happened
> >> to convert the file to the format matching the suffix?  Or would this
> >> start giving warnings if the known suffix doesn't match the probed contents?
> >>
> > 
> > (Eric, I should have cc'ed you on my last email, sorry)
> > 
> > Image this scenario:
> > 
> >     existing chain created a while ago, via:
> > 
> >         qemu-img create -f qcow2 foo.img 1G
> >         qemu-img create -f qcow2 -b foo.img bar.img 1G
> > 
> 
> Libvirt refuses to create files without backing format encoded; but is
> still stuck dealing with files created externally (such as qemu-img
> create) where it was forgotten.
> 
> > 
> > User launches qemu by this commandline:
> > 
> >     qemu-kvm -drive file=bar.img,format=qcow2
> > 
> > 
> > Old behavior:
> > 
> >   | foo.img | <--- | bar.img |
> >   | (qcow2) |      | (qcow2) |
> > 
> > New behavior:
> > 
> >   | foo.img | <--- | bar.img |
> >   | (raw)   |      | (qcow2) |
> 
> Libvirt already assumed raw for any image where encoding was not
> specified, so this scenario is already a risky one for the user.
>

Right, but not all QEMU users are libvirt users.  And for standalone
QEMU users, it would be a behavior change, and one that leads to data
corruption.

> > 
> > 
> > So I think we want to make sure that we don't just fall back to raw
> > for unknown filename extensions.
> 
> Remember, if a probe determines a file is raw, it is safe to use as raw.
>  Falling back to raw is IDEAL.

I agree that, from the perspective of host security, assuming raw is
always safer.  However, that does NOT mean the ideal solution is to
parse filenames, and create fallbacks to raw that didn't happen
before.  I think the better scenario there is to decide we don't know
what we can do, and exit.

Option 1: Probe as usual, detect format.  Host UNSAFE.
Option 2: Probe differently, fall back to raw more aggressively. 
          Host SAFE, Guest may have data loss.  Requires rebase
          of backing chain to prevent data loss.
Option 3: Exit, and demand to be told the image format type.
          Host SAFE, Guest has no data loss.  Inconvenient; requires
          rebasing backing chain to open.
Option 4: Kevin's raw write prevent.  Not sure all the ramifications
          of this.

> 
> What _might_ be nice is teaching qemu-img to auto-encode the backing
> format during 'qemu-img create' (it can be assumed that _at creation
> time_, the backing file hasn't had a chance to be corrupted by the guest
> yet) rather than the current behavior of only encoding an explicit
> format given by the user.  

I agree qemu-img could use some change. When you look at the commandline 
'qemu-img create -f qcow2 bar.img -b foo.img', it is not
immediately intuitive that you should also have specified '-F qcow2'.
But I am not sure that it safe to assume that the backing file has not
been corrupted by the guest at this point!  The file foo.img may have
been around and in use for a while.  I think qemu-img should warn
loudly the backing format was not specified, and that this may be
unsafe.

>Furthermore, an explicit encoding from the
> user should ALWAYS be trusted, and the user can override any place where
> automatic heuristics would have guessed differently (whether those
> heuristics were based on probing, file extension, or otherwise).
> 

Definitely agree here.