[Qemu-devel] RFC configurable block formats

[Qemu-devel] RFC configurable block formats

[Markus Armbruster]

We have code for a quite a few block formats.  A quick grep shows bochs,
cloop, cow, dmg, ftp, ftps, host_cdrom, host_device, host_floppy, http,
https, nbd, parallels, qcow, qcow2, raw, tftp, vdi, vmdk, vpc, vvfat.
Only formats ftp, ftps, http, https, tftp are optional (see configure
--enable-curl).

While I trust that all of these formats are useful at least for some
people in some circumstances, some of them are of a kind that friends
don't let friends use in production.

In short, I'd like to be able to configure block formats.  Simple
enough, eh?  Except there's a catch: I'd like to be able to include more
formats in tools like qemu-img than in qemu proper.  That lets me
restrict qemu proper, where a faulty block driver has the greatest
potential for mischief, to the formats I trust and need, and still keep
useful capability for other formats in qemu-img.

Thus, I'd like to be able to configure a block driver off for
everything, or on for utility programs and off for qemu proper, or on
for everything.

A naive implementation of this idea simply links only those block
drivers into a program that have been configured for it.  Unfortunately,
this leads to an unwanted difference in behavior between the different
programs when the format is probed.

Probing gives every block driver a chance to "score" the image, and
picks the one with the highest score (I'm simplifying, but it'll do to
illustrate the problem).  If two programs have different sets of
drivers, probing may yield different results.  I don't like that.

Say I configure crusty old qcow (note lack of 2) for utility programs
only.  Then I don't want qemu to silently treat qcow images as raw, I
want it to tell me it can't do qcow.  To be precise:

If a format is configured off, no program shall recognize it.

Else all programs shall recognize it, but

    if it is configured on for utility programs, off for qemu proper,
    then recognizing it in qemu proper shall be an error.

If you agree this is useful, I'd be willing to code it.

Re: [Qemu-devel] RFC configurable block formats

[Anthony Liguori]

Markus Armbruster wrote:
> We have code for a quite a few block formats.  A quick grep shows bochs,
> cloop, cow, dmg, ftp, ftps, host_cdrom, host_device, host_floppy, http,
> https, nbd, parallels, qcow, qcow2, raw, tftp, vdi, vmdk, vpc, vvfat.
> Only formats ftp, ftps, http, https, tftp are optional (see configure
> --enable-curl).
>
> While I trust that all of these formats are useful at least for some
> people in some circumstances, some of them are of a kind that friends
> don't let friends use in production.
>
> In short, I'd like to be able to configure block formats.  Simple
> enough, eh?  Except there's a catch: I'd like to be able to include more
> formats in tools like qemu-img than in qemu proper.  That lets me
> restrict qemu proper, where a faulty block driver has the greatest
> potential for mischief, to the formats I trust and need, and still keep
> useful capability for other formats in qemu-img.
>
> Thus, I'd like to be able to configure a block driver off for
> everything, or on for utility programs and off for qemu proper, or on
> for everything.
>
> A naive implementation of this idea simply links only those block
> drivers into a program that have been configured for it.  Unfortunately,
> this leads to an unwanted difference in behavior between the different
> programs when the format is probed.
>
> Probing gives every block driver a chance to "score" the image, and
> picks the one with the highest score (I'm simplifying, but it'll do to
> illustrate the problem).  If two programs have different sets of
> drivers, probing may yield different results.  I don't like that.
>
> Say I configure crusty old qcow (note lack of 2) for utility programs
> only.  Then I don't want qemu to silently treat qcow images as raw, I
> want it to tell me it can't do qcow.  To be precise:
>
> If a format is configured off, no program shall recognize it.
>
> Else all programs shall recognize it, but
>
>     if it is configured on for utility programs, off for qemu proper,
>     then recognizing it in qemu proper shall be an error.
>
> If you agree this is useful, I'd be willing to code it.
>   

I'd rather see something like a driver white list/black list for qemu 
proper.  The list would be used to exclude block formats and could be 
extended to support read-only formats vs. read-write formats.  For 
instance, --enable-block-formats='qcow2 raw'.  It avoids polluting the 
block interface with knowledge of the distinction between a "utility" 
program and qemu proper.

Regards,

Anthony Liguori

Re: [Qemu-devel] RFC configurable block formats

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster wrote:
>> We have code for a quite a few block formats.  A quick grep shows bochs,
>> cloop, cow, dmg, ftp, ftps, host_cdrom, host_device, host_floppy, http,
>> https, nbd, parallels, qcow, qcow2, raw, tftp, vdi, vmdk, vpc, vvfat.
>> Only formats ftp, ftps, http, https, tftp are optional (see configure
>> --enable-curl).
>>
>> While I trust that all of these formats are useful at least for some
>> people in some circumstances, some of them are of a kind that friends
>> don't let friends use in production.
>>
>> In short, I'd like to be able to configure block formats.  Simple
>> enough, eh?  Except there's a catch: I'd like to be able to include more
>> formats in tools like qemu-img than in qemu proper.  That lets me
>> restrict qemu proper, where a faulty block driver has the greatest
>> potential for mischief, to the formats I trust and need, and still keep
>> useful capability for other formats in qemu-img.
>>
>> Thus, I'd like to be able to configure a block driver off for
>> everything, or on for utility programs and off for qemu proper, or on
>> for everything.
>>
>> A naive implementation of this idea simply links only those block
>> drivers into a program that have been configured for it.  Unfortunately,
>> this leads to an unwanted difference in behavior between the different
>> programs when the format is probed.
>>
>> Probing gives every block driver a chance to "score" the image, and
>> picks the one with the highest score (I'm simplifying, but it'll do to
>> illustrate the problem).  If two programs have different sets of
>> drivers, probing may yield different results.  I don't like that.
>>
>> Say I configure crusty old qcow (note lack of 2) for utility programs
>> only.  Then I don't want qemu to silently treat qcow images as raw, I
>> want it to tell me it can't do qcow.  To be precise:
>>
>> If a format is configured off, no program shall recognize it.
>>
>> Else all programs shall recognize it, but
>>
>>     if it is configured on for utility programs, off for qemu proper,
>>     then recognizing it in qemu proper shall be an error.
>>
>> If you agree this is useful, I'd be willing to code it.
>>   
>
> I'd rather see something like a driver white list/black list for qemu
> proper.

Actually, that's what I had in mind for implementation.

>          The list would be used to exclude block formats and could be
> extended to support read-only formats vs. read-write formats.  For
> instance, --enable-block-formats='qcow2 raw'.  It avoids polluting the
> block interface with knowledge of the distinction between a "utility"
> program and qemu proper.

I agree it's better to keep it out of the BlockDriver interface.

Here's a patch for illustration.  It's only lightly tested, lacks the
configure part, and I still need to check all users of bdrv_open2() are
happy.  Are we on the same page?



diff --git a/block.c b/block.c
index 33f3d65..fef686f 100644
--- a/block.c
+++ b/block.c
@@ -61,6 +61,9 @@ BlockDriverState *bdrv_first;
 
 static BlockDriver *first_drv;
 
+/* If non-zero, use only whitelisted block drivers */
+static int use_bdrv_whitelist;
+
 int path_is_absolute(const char *path)
 {
     const char *p;
@@ -171,6 +174,28 @@ BlockDriver *bdrv_find_format(const char *format_name)
     return NULL;
 }
 
+static int bdrv_is_whitelisted(BlockDriver *drv)
+{
+    static const char *whitelist[] = {
+        /* TODO this needs to come from configure */
+        "raw", "qcow2", "host_device", NULL
+    };
+    const char **p;
+
+    for (p = whitelist; *p; p++) {
+        if (!strcmp(drv->format_name, *p)) {
+            return 1;
+        }
+    }
+    return 0;
+}
+
+BlockDriver *bdrv_find_whitelisted_format(const char *format_name)
+{
+    BlockDriver *drv = bdrv_find_format(format_name);
+    return drv && bdrv_is_whitelisted(drv) ? drv : NULL;
+}
+
 int bdrv_create(BlockDriver *drv, const char* filename,
     QEMUOptionParameter *options)
 {
@@ -427,7 +452,10 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 		(flags & (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
     else
         open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
-    ret = drv->bdrv_open(bs, filename, open_flags);
+    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv))
+        ret = -ENOTSUP;
+    else
+        ret = drv->bdrv_open(bs, filename, open_flags);
     if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
         ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
         bs->read_only = 1;
@@ -1739,6 +1767,12 @@ void bdrv_init(void)
     module_call_init(MODULE_INIT_BLOCK);
 }
 
+void bdrv_init_with_whitelist(void)
+{
+    use_bdrv_whitelist = 1;
+    bdrv_init();
+}
+
 void *qemu_aio_get(AIOPool *pool, BlockDriverState *bs,
                    BlockDriverCompletionFunc *cb, void *opaque)
 {
diff --git a/block.h b/block.h
index a966afb..91c7daf 100644
--- a/block.h
+++ b/block.h
@@ -45,7 +45,9 @@ void bdrv_info(Monitor *mon);
 void bdrv_info_stats(Monitor *mon);
 
 void bdrv_init(void);
+void bdrv_init_with_whitelist(void);
 BlockDriver *bdrv_find_format(const char *format_name);
+BlockDriver *bdrv_find_whitelisted_format(const char *format_name);
 int bdrv_create(BlockDriver *drv, const char* filename,
     QEMUOptionParameter *options);
 int bdrv_create2(BlockDriver *drv,
diff --git a/monitor.c b/monitor.c
index f105a2e..124123b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -482,7 +482,7 @@ static void do_change_block(Monitor *mon, const char *device,
         return;
     }
     if (fmt) {
-        drv = bdrv_find_format(fmt);
+        drv = bdrv_find_whitelisted_format(fmt);
         if (!drv) {
             monitor_printf(mon, "invalid format %s\n", fmt);
             return;
diff --git a/vl.c b/vl.c
index 7bfd415..580e4a5 100644
--- a/vl.c
+++ b/vl.c
@@ -2062,7 +2062,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
             fprintf(stderr, "\n");
 	    return NULL;
         }
-        drv = bdrv_find_format(buf);
+        drv = bdrv_find_whitelisted_format(buf);
         if (!drv) {
             fprintf(stderr, "qemu: '%s' invalid format\n", buf);
             return NULL;
@@ -5597,7 +5597,7 @@ int main(int argc, char **argv, char **envp)
     /* init the dynamic translator */
     cpu_exec_init_all(tb_size * 1024 * 1024);
 
-    bdrv_init();
+    bdrv_init_with_whitelist();
 
     /* we always create the cdrom drive, even if no disk is there */
     drive_add(NULL, CDROM_ALIAS);

Re: [Qemu-devel] RFC configurable block formats

[Anthony Liguori]

Markus Armbruster wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>   
>
>>          The list would be used to exclude block formats and could be
>> extended to support read-only formats vs. read-write formats.  For
>> instance, --enable-block-formats='qcow2 raw'.  It avoids polluting the
>> block interface with knowledge of the distinction between a "utility"
>> program and qemu proper.
>>     
>
> I agree it's better to keep it out of the BlockDriver interface.
>
> Here's a patch for illustration.  It's only lightly tested, lacks the
> configure part, and I still need to check all users of bdrv_open2() are
> happy.  Are we on the same page?
>   

Looks reasonable to me.

Regards,

Anthony Liguori

Re: [Qemu-devel] RFC configurable block formats

[Gerd Hoffmann]

> +static int bdrv_is_whitelisted(BlockDriver *drv)
> +{
> +    static const char *whitelist[] = {
> +        /* TODO this needs to come from configure */
> +        "raw", "qcow2", "host_device", NULL
> +    };

Maybe have separate whitelists for read/write and readonly access?
So we could -- say -- allow readonly access to qcow images?

cheers,
   Gerd