[PATCH] block: Add option to use driver whitelist even in tools

[PATCH] block: Add option to use driver whitelist even in tools

[Kevin Wolf]

Currently, the block driver whitelists are only applied for the system
emulator. All other binaries still give unrestricted access to all block
drivers. There are use cases where this made sense because the main
concern was avoiding customers running VMs on less optimised block
drivers and getting bad performance. Allowing the same image format e.g.
as a target for 'qemu-img convert' is not a problem then.

However, if the concern is the supportability of the driver in general,
either in full or when used read-write, not applying the list driver
whitelist in tools doesn't help - especially since qemu-nbd and
qemu-storage-daemon now give access to more or less the same operations
in block drivers as running a system emulator.

In order to address this, introduce a new configure option that enforces
the driver whitelist in all binaries.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 configure   | 14 ++++++++++++--
 block.c     |  3 +++
 meson.build |  1 +
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 650d9c0735..d3f2a362d5 100755
--- a/configure
+++ b/configure
@@ -243,6 +243,7 @@ cross_prefix=""
 audio_drv_list=""
 block_drv_rw_whitelist=""
 block_drv_ro_whitelist=""
+block_drv_whitelist_tools="no"
 host_cc="cc"
 audio_win_int=""
 libs_qga=""
@@ -1003,6 +1004,10 @@ for opt do
   ;;
   --block-drv-ro-whitelist=*) block_drv_ro_whitelist=$(echo "$optarg" | sed -e 's/,/ /g')
   ;;
+  --enable-block-drv-whitelist-in-tools) block_drv_whitelist_tools="yes"
+  ;;
+  --disable-block-drv-whitelist-in-tools) block_drv_whitelist_tools="no"
+  ;;
   --enable-debug-tcg) debug_tcg="yes"
   ;;
   --disable-debug-tcg) debug_tcg="no"
@@ -1776,10 +1781,12 @@ Advanced options (experts only):
   --block-drv-whitelist=L  Same as --block-drv-rw-whitelist=L
   --block-drv-rw-whitelist=L
                            set block driver read-write whitelist
-                           (affects only QEMU, not qemu-img)
+                           (by default affects only QEMU, not tools like qemu-img)
   --block-drv-ro-whitelist=L
                            set block driver read-only whitelist
-                           (affects only QEMU, not qemu-img)
+                           (by default affects only QEMU, not tools like qemu-img)
+  --enable-block-drv-whitelist-in-tools
+                           use block whitelist also in tools instead of only QEMU
   --enable-trace-backends=B Set trace backend
                            Available backends: $trace_backend_list
   --with-trace-file=NAME   Full PATH,NAME of file to store traces
@@ -4556,6 +4563,9 @@ if test "$audio_win_int" = "yes" ; then
 fi
 echo "CONFIG_BDRV_RW_WHITELIST=$block_drv_rw_whitelist" >> $config_host_mak
 echo "CONFIG_BDRV_RO_WHITELIST=$block_drv_ro_whitelist" >> $config_host_mak
+if test "$block_drv_whitelist_tools" = "yes" ; then
+  echo "CONFIG_BDRV_WHITELIST_TOOLS=y" >> $config_host_mak
+fi
 if test "$xfs" = "yes" ; then
   echo "CONFIG_XFS=y" >> $config_host_mak
 fi
diff --git a/block.c b/block.c
index be083f389e..e97ce0b1c8 100644
--- a/block.c
+++ b/block.c
@@ -6162,6 +6162,9 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
 
 void bdrv_init(void)
 {
+#ifdef CONFIG_BDRV_WHITELIST_TOOLS
+    use_bdrv_whitelist = 1;
+#endif
     module_call_init(MODULE_INIT_BLOCK);
 }
 
diff --git a/meson.build b/meson.build
index eb362ee5eb..9b41b9e6d5 100644
--- a/meson.build
+++ b/meson.build
@@ -2859,6 +2859,7 @@ summary_info += {'coroutine pool':    config_host['CONFIG_COROUTINE_POOL'] == '1
 if have_block
   summary_info += {'Block whitelist (rw)': config_host['CONFIG_BDRV_RW_WHITELIST']}
   summary_info += {'Block whitelist (ro)': config_host['CONFIG_BDRV_RO_WHITELIST']}
+  summary_info += {'Use block whitelist in tools': config_host.has_key('CONFIG_BDRV_WHITELIST_TOOLS')}
   summary_info += {'VirtFS support':    have_virtfs}
   summary_info += {'build virtiofs daemon': have_virtiofsd}
   summary_info += {'Live block migration': config_host.has_key('CONFIG_LIVE_BLOCK_MIGRATION')}
-- 
2.31.1

Re: [PATCH] block: Add option to use driver whitelist even in tools

[Eric Blake]

On Fri, Jul 09, 2021 at 06:41:41PM +0200, Kevin Wolf wrote:
> Currently, the block driver whitelists are only applied for the system
> emulator. All other binaries still give unrestricted access to all block
> drivers. There are use cases where this made sense because the main
> concern was avoiding customers running VMs on less optimised block
> drivers and getting bad performance. Allowing the same image format e.g.
> as a target for 'qemu-img convert' is not a problem then.
> 
> However, if the concern is the supportability of the driver in general,
> either in full or when used read-write, not applying the list driver
> whitelist in tools doesn't help - especially since qemu-nbd and
> qemu-storage-daemon now give access to more or less the same operations
> in block drivers as running a system emulator.
> 
> In order to address this, introduce a new configure option that enforces
> the driver whitelist in all binaries.

Is it feasible that someone would want two separate lists: one for
qemu (which runs run efficiently) and another for tools (which ones do
we support at all)?  As written, your patch offers no chance to
distinguish between the two.

Also, is now a good time to join the bandwagon on picking a more
descriptive name (such as 'allow-list') for this terminology?

> +++ b/block.c
> @@ -6162,6 +6162,9 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>  
>  void bdrv_init(void)
>  {
> +#ifdef CONFIG_BDRV_WHITELIST_TOOLS
> +    use_bdrv_whitelist = 1;

Pre-existing, but this variable seems like a natural candidate to be
bool instead of int.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] block: Add option to use driver whitelist even in tools

[Kevin Wolf]

Am 09.07.2021 um 19:45 hat Eric Blake geschrieben:
> On Fri, Jul 09, 2021 at 06:41:41PM +0200, Kevin Wolf wrote:
> > Currently, the block driver whitelists are only applied for the system
> > emulator. All other binaries still give unrestricted access to all block
> > drivers. There are use cases where this made sense because the main
> > concern was avoiding customers running VMs on less optimised block
> > drivers and getting bad performance. Allowing the same image format e.g.
> > as a target for 'qemu-img convert' is not a problem then.
> > 
> > However, if the concern is the supportability of the driver in general,
> > either in full or when used read-write, not applying the list driver
> > whitelist in tools doesn't help - especially since qemu-nbd and
> > qemu-storage-daemon now give access to more or less the same operations
> > in block drivers as running a system emulator.
> > 
> > In order to address this, introduce a new configure option that enforces
> > the driver whitelist in all binaries.
> 
> Is it feasible that someone would want two separate lists: one for
> qemu (which runs run efficiently) and another for tools (which ones do
> we support at all)?  As written, your patch offers no chance to
> distinguish between the two.

Possibly. However, supporting a second list would require a much larger
code change than this patch, so I'd say this is a problem we should only
solve when someone actually has it.

> Also, is now a good time to join the bandwagon on picking a more
> descriptive name (such as 'allow-list') for this terminology?

I don't have an opinion on the time, but I do have an opinion on using a
separate email thread for it. :-)

Initially I tried to find a way not to use "whitelist" in the new option
name, but that only made things inconsistent and confusing, and renaming
the existing options is definitely out of scope for this patch.

> > +++ b/block.c
> > @@ -6162,6 +6162,9 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
> >  
> >  void bdrv_init(void)
> >  {
> > +#ifdef CONFIG_BDRV_WHITELIST_TOOLS
> > +    use_bdrv_whitelist = 1;
> 
> Pre-existing, but this variable seems like a natural candidate to be
> bool instead of int.

Yes, I guess we could have a cleanup patch there.

Kevin

Re: [PATCH] block: Add option to use driver whitelist even in tools

[Eric Blake]

On Mon, Jul 12, 2021 at 10:18:30AM +0200, Kevin Wolf wrote:
> > Is it feasible that someone would want two separate lists: one for
> > qemu (which runs run efficiently) and another for tools (which ones do
> > we support at all)?  As written, your patch offers no chance to
> > distinguish between the two.
> 
> Possibly. However, supporting a second list would require a much larger
> code change than this patch, so I'd say this is a problem we should only
> solve when someone actually has it.

Indeed.

> 
> > Also, is now a good time to join the bandwagon on picking a more
> > descriptive name (such as 'allow-list') for this terminology?
> 
> I don't have an opinion on the time, but I do have an opinion on using a
> separate email thread for it. :-)

Agreed with that sentiment.

> 
> Initially I tried to find a way not to use "whitelist" in the new option
> name, but that only made things inconsistent and confusing, and renaming
> the existing options is definitely out of scope for this patch.

Also agreed.  Therefore, reviewing this patch in isolation (even if we
eventually do followups for the issues I pointed out) is fine, and you
now have:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] block: Add option to use driver whitelist even in tools

[Daniel P. Berrangé]

On Mon, Jul 12, 2021 at 10:18:30AM +0200, Kevin Wolf wrote:
> Am 09.07.2021 um 19:45 hat Eric Blake geschrieben:
> > On Fri, Jul 09, 2021 at 06:41:41PM +0200, Kevin Wolf wrote:
> > > Currently, the block driver whitelists are only applied for the system
> > > emulator. All other binaries still give unrestricted access to all block
> > > drivers. There are use cases where this made sense because the main
> > > concern was avoiding customers running VMs on less optimised block
> > > drivers and getting bad performance. Allowing the same image format e.g.
> > > as a target for 'qemu-img convert' is not a problem then.
> > > 
> > > However, if the concern is the supportability of the driver in general,
> > > either in full or when used read-write, not applying the list driver
> > > whitelist in tools doesn't help - especially since qemu-nbd and
> > > qemu-storage-daemon now give access to more or less the same operations
> > > in block drivers as running a system emulator.
> > > 
> > > In order to address this, introduce a new configure option that enforces
> > > the driver whitelist in all binaries.
> > 
> > Is it feasible that someone would want two separate lists: one for
> > qemu (which runs run efficiently) and another for tools (which ones do
> > we support at all)?  As written, your patch offers no chance to
> > distinguish between the two.
> 
> Possibly. However, supporting a second list would require a much larger
> code change than this patch, so I'd say this is a problem we should only
> solve when someone actually has it.
> 
> > Also, is now a good time to join the bandwagon on picking a more
> > descriptive name (such as 'allow-list') for this terminology?
> 
> I don't have an opinion on the time, but I do have an opinion on using a
> separate email thread for it. :-)

It isn't difficult - the word "white" adds no value in this arg
and can simply be removed entirely right now.
 
> Initially I tried to find a way not to use "whitelist" in the new option
> name, but that only made things inconsistent and confusing, and renaming
> the existing options is definitely out of scope for this patch.

Calling it --block-drv-list, would be consistent with the existing
--audio-drv-list. Fixing up the other existing block list args would
be nice, but should not stop use of the better name in this patch
right now.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|