Re: [Qemu-devel] RFC configurable block formats

[Markus Armbruster <armbru@redhat.com>]

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