[V1,24/26] seccomp: cpr-exec blocker

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(-)

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