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

[Corey Bryant]

On 12/09/2013 12:51 PM, Eduardo Otubo wrote:
> On 12/09/2013 03:33 PM, Daniel P. Berrange wrote:
> > 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.
>
> I think an admin must know what he's doing. If he requested sandbox but
> without kernel support he need to step back a little and understand what
> he's doing. This patch won't decrease the security level, IMHO.

Preventing qemu from exiting is definitely not the right approach. 
Please feel free to run code by me ahead of time in the future.

> > 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.
>
> But I think this could be done on virt-test as well :)

This would make sense.

Although it sounds like Lucas was looking for an error message when 
seccomp kills qemu.  Maybe virt-test could grep the audit log for the 
existence of a "type=SECCOMP" record within the test's time of 
execution, and issue a message based on that.

--
Regards,
Corey Bryant