Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Thomas Huth" <thuth@redhat.com>,
	"Prasad J Pandit" <pjp@fedoraproject.org>,
	qemu-block@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Markus Armbruster" <armbru@redhat.com>,
	qemu-devel@nongnu.org, xen-devel@lists.xenproject.org,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>
Subject: Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API
Date: Thu, 9 Sep 2021 11:55:09 +0100	[thread overview]
Message-ID: <YTnoDYpDaj055PnS@redhat.com> (raw)
In-Reply-To: <YTnkhxWbm3NvGo/T@redhat.com>

On Thu, Sep 09, 2021 at 11:40:07AM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> > Add the BlockDriver::bdrv_taints_security_policy() handler.
> > Drivers implementing it might taint the global QEMU security
> > policy.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  include/block/block_int.h | 6 +++++-
> >  block.c                   | 6 ++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index f1a54db0f8c..0ec0a5c06e9 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -169,7 +169,11 @@ struct BlockDriver {
> >      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
> >                            Error **errp);
> >      void (*bdrv_close)(BlockDriverState *bs);
> > -
> > +    /*
> > +     * Return %true if the driver is withing QEMU security policy boundary,
> > +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> > +     */
> > +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);

Also as with previous comments, I think we should not refer
to tainting or the security policy here, but instead simply
document whether we consider the bdrv to be secure or not.

Tainting is merely one action that is taken in accordance with
the security policy, as a result of the information presented.

> >      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> >                                         Error **errp);
> > diff --git a/block.c b/block.c
> > index b2b66263f9a..696ba486001 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -49,6 +49,7 @@
> >  #include "qemu/timer.h"
> >  #include "qemu/cutils.h"
> >  #include "qemu/id.h"
> > +#include "qemu-common.h"
> >  #include "block/coroutines.h"
> >  
> >  #ifdef CONFIG_BSD
> > @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
> >          }
> >      }
> >  
> > +    if (drv->bdrv_taints_security_policy) {
> > +        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> > +                                   "Block protocol '%s'", drv->format_name);
> > +    }
> > +
> >      return 0;
> >  open_failed:
> >      bs->drv = NULL;
> 
> Again we need a way to report this via QAPI, but we don't have a natural
> place is hang this off for introspection before starting a guest.
> 
> The best we can do is report the information after a block backend has
> been instantiated. eg  Modify "BlockInfo" struct to gain
> 
>    '*secure': 'bool'
> 
> Note I made this an optional field, since unless we mark every single
> block driver impl straight away, we'll need to cope with the absence
> of information.

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