| Message ID | 20210908232024.2399215-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: Introduce qemu_security_policy_taint() API | expand |
On 9/9/21 1:20 AM, 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); > > 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); Hmm I should check for phase_check(PHASE_MACHINE_READY) and qemu_security_policy_is_strict() somehow, to refuse adding unsafe drv at runtime instead of exiting... > + } > + > return 0; > open_failed: > bs->drv = NULL; >
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); > > 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
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
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, within > + * %false otherwise. See: https://www.qemu.org/contribute/security-process/ > + */ > + bool (*bdrv_taints_security_policy)(BlockDriverState *bs); > > int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts, > Error **errp);
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); 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;
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(-)