Message ID | 1714406135-451286-25-git-send-email-steven.sistare@oracle.com |
---|---|
State | New |
Headers | show |
Series | Live update: cpr-exec | expand |
Steve Sistare <steven.sistare@oracle.com> writes: > cpr-exec mode needs permission to exec. Block it if permission is denied. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
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.sistare@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. With regards, Daniel
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.sistare@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. Would it be more palatable if I move this blocker's creation to cpr_mig_init? - Steve
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.sistare@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
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); + } } #endif
cpr-exec mode needs permission to exec. Block it if permission is denied. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/sysemu/seccomp.h | 1 + system/qemu-seccomp.c | 10 ++++++++-- system/vl.c | 6 ++++++ 3 files changed, 15 insertions(+), 2 deletions(-)