On Mon, May 13, 2024 at 03:29:48PM -0400, Steven Sistare wrote:
> On 5/10/2024 3:54 AM, Daniel P. Berrangé wrote:
> > On Mon, Apr 29, 2024 at 08:55:33AM -0700, Steve Sistare wrote:
> > > cpr-exec mode needs permission to exec. Block it if permission is denied.
> > >
> > > Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
> > > ---
> > > include/sysemu/seccomp.h | 1 +
> > > system/qemu-seccomp.c | 10 ++++++++--
> > > system/vl.c | 6 ++++++
> > > 3 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h
> > > index fe85989..023c0a1 100644
> > > --- a/include/sysemu/seccomp.h
> > > +++ b/include/sysemu/seccomp.h
> > > @@ -22,5 +22,6 @@
> > > #define QEMU_SECCOMP_SET_RESOURCECTL (1 << 4)
> > > int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp);
> > > +uint32_t qemu_seccomp_get_opts(void);
> > > #endif
> > > diff --git a/system/qemu-seccomp.c b/system/qemu-seccomp.c
> > > index 5c20ac0..0d2a561 100644
> > > --- a/system/qemu-seccomp.c
> > > +++ b/system/qemu-seccomp.c
> > > @@ -360,12 +360,18 @@ static int seccomp_start(uint32_t seccomp_opts,
> > > Error **errp)
> > > return rc < 0 ? -1 : 0;
> > > }
> > > +static uint32_t seccomp_opts;
> > > +
> > > +uint32_t qemu_seccomp_get_opts(void)
> > > +{
> > > + return seccomp_opts;
> > > +}
> > > +
> > > int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
> > > {
> > > if (qemu_opt_get_bool(opts, "enable", false)) {
> > > - uint32_t seccomp_opts = QEMU_SECCOMP_SET_DEFAULT
> > > - | QEMU_SECCOMP_SET_OBSOLETE;
> > > const char *value = NULL;
> > > + seccomp_opts = QEMU_SECCOMP_SET_DEFAULT |
> > > QEMU_SECCOMP_SET_OBSOLETE;
> > > value = qemu_opt_get(opts, "obsolete");
> > > if (value) {
> > > diff --git a/system/vl.c b/system/vl.c
> > > index 7252100..b76881e 100644
> > > --- a/system/vl.c
> > > +++ b/system/vl.c
> > > @@ -76,6 +76,7 @@
> > > #include "hw/block/block.h"
> > > #include "hw/i386/x86.h"
> > > #include "hw/i386/pc.h"
> > > +#include "migration/blocker.h"
> > > #include "migration/cpr.h"
> > > #include "migration/misc.h"
> > > #include "migration/snapshot.h"
> > > @@ -2493,6 +2494,11 @@ static void qemu_process_early_options(void)
> > > QemuOptsList *olist = qemu_find_opts_err("sandbox", NULL);
> > > if (olist) {
> > > qemu_opts_foreach(olist, parse_sandbox, NULL, &error_fatal);
> > > + if (qemu_seccomp_get_opts() & QEMU_SECCOMP_SET_SPAWN) {
> > > + Error *blocker = NULL;
> > > + error_setg(&blocker, "-sandbox denies exec for cpr-exec");
> > > + migrate_add_blocker_mode(&blocker, MIG_MODE_CPR_EXEC,
> > > &error_fatal);
> > > + }
> > > }
> > > #endi
> >
> > There are a whole pile of features that get blocked wehn -sandbox is
> > used. I'm not convinced we should be adding code to check for specific
> > blocked features, as such a list will always be incomplete at best, and
> > incorrectly block things at worst.
> >
> > I view this primarily as a documentation task for the cpr-exec command.
>
> For cpr and live migration, we do our best to prevent breaking the guest
> for cases we know will fail. Independently, a clear error message here
> will reduce error reports for this new cpr feature.
I would expect the QMP command that triggers the sandbox to report a
clear error when getting EPERM.
> Would it be more palatable if I move this blocker's creation to cpr_mig_init?
Not particularly, as I don't think other code in QEMU should be looking
at the sandbox impl.
With 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 :|
