Re: [Qemu-devel] [PATCH] seccomp: "-sandbox on" won't kill Qemu when option not built in

[Daniel P. Berrange]

On Mon, Dec 09, 2013 at 03:20:52PM -0200, Eduardo Otubo wrote:
> This option was requested by virt-test team so they can run tests with
> Qemu and "-sandbox on" set without breaking whole test if host doesn't
> have support for seccomp in kernel. It covers two possibilities:
> 
>  1) Host kernel support does not support seccomp, but user installed Qemu 
>     package with sandbox support: Libseccomp will fail -> qemu will fail 
>     nicely and won't stop execution.
> 
>  2) Host kernel has support but Qemu package wasn't built with sandbox
>     feature. Qemu will fail nicely and won't stop execution.
> 
> Signed-off-by: Eduardo Otubo <address@hidden>
> ---
>  vl.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index b0399de..a0806dc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -967,13 +967,11 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>  #ifdef CONFIG_SECCOMP
>          if (seccomp_start() < 0) {
>              qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -                          "failed to install seccomp syscall filter in the 
> kernel");
> -            return -1;
> +                          "failed to install seccomp syscall filter in the 
> kernel, disabling it");
>          }
>  #else
>          qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -                      "sandboxing request but seccomp is not compiled into 
> this build");
> -        return -1;
> +                      "sandboxing request but seccomp is not compiled into 
> this build, disabling it");
>  #endif
>      }
>  
> @@ -3808,9 +3806,7 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 
> 0)) {
> -        exit(1);
> -    }
> +    qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0);
>  
>  #ifndef _WIN32
>      if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) {

This change is really dubious from a security POV. If the admin requested
sandboxing and the host or QEMU build cannot support it, then QEMU really
*must* exit.

IMHO the test suite should probe to see if sandbox is working or not, and
just not use the "-sandbox on" arg if the host doesn't support it.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|