Re: [Qemu-devel] [PATCH] sandbox: Report error on forbidden system call

[Corey Bryant]

On 02/06/2013 06:13 AM, Michal Privoznik wrote:
> On 05.02.2013 15:28, Corey Bryant wrote:
> > On 02/05/2013 06:02 AM, Michal Privoznik wrote:
> > > Currently, it we call a not white listed system call, we get killed
> > > immediately without reporting any error. It would be far more useful,
> > > if we can at least shout something on stderr just before dying, so
> > > users know it is because of sandbox, not just random quit.
> > >
> > > Signed-off-by: Michal Privoznik <address@hidden>
> > > ---
> > >    os-posix.c     | 8 ++++++++
> > >    qemu-seccomp.c | 4 +++-
> > >    2 files changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/os-posix.c b/os-posix.c
> > > index 5c64518..1d52306 100644
> > > --- a/os-posix.c
> > > +++ b/os-posix.c
> > > @@ -62,6 +62,12 @@ void os_setup_early_signal_handling(void)
> > >        sigaction(SIGPIPE, &act, NULL);
> > >    }
> > >
> > > +static void syssig_handler(int signal, siginfo_t *info, void *c)
> > > +{
> > > +    fprintf(stderr, "Bad system call\n");
> > > +    exit(1);
> > > +}
> > > +
> > >    static void termsig_handler(int signal, siginfo_t *info, void *c)
> > >    {
> > >        qemu_system_killed(info->si_signo, info->si_pid);
> > > @@ -77,6 +83,8 @@ void os_setup_signal_handling(void)
> > >        sigaction(SIGINT,  &act, NULL);
> > >        sigaction(SIGHUP,  &act, NULL);
> > >        sigaction(SIGTERM, &act, NULL);
> > > +    act.sa_sigaction = syssig_handler;
> > > +    sigaction(SIGSYS,  &act, NULL);
> > >    }
> > >
> > >    /* Find a likely location for support files using the location of
> > > the binary.
> > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > > index 031da1d..897d9b3 100644
> > > --- a/qemu-seccomp.c
> > > +++ b/qemu-seccomp.c
> > > @@ -2,9 +2,11 @@
> > >     * QEMU seccomp mode 2 support with libseccomp
> > >     *
> > >     * Copyright IBM, Corp. 2012
> > > + * Copyright (C) 2013 Red Hat, Inc.
> > >     *
> > >     * Authors:
> > >     *  Eduardo Otubo    <address@hidden>
> > > + *  Michal Privoznik <address@hidden>
> > >     *
> > >     * This work is licensed under the terms of the GNU GPL, version 2.
> > > See
> > >     * the COPYING file in the top-level directory.
> > > @@ -238,7 +240,7 @@ int seccomp_start(void)
> > >        unsigned int i = 0;
> > >        scmp_filter_ctx ctx;
> > >
> > > -    ctx = seccomp_init(SCMP_ACT_KILL);
> > > +    ctx = seccomp_init(SCMP_ACT_TRAP);
> > >        if (ctx == NULL) {
> > >            goto seccomp_return;
> > >        }
> >
> > I think this is going to be better solved in the kernel.  I have a
> > kernel patch sitting out there at: https://lkml.org/lkml/2013/1/7/313
> > Any public support of this patch could be useful to help get it in.
> >
> > Something is definitely needed to learn the syscall that is killing
> > QEMU.  But I don't think the signal handler approach is going to work.
> > We tried that and ran into too many situations where signals were being
> > blocked by libraries (spice is one example).  And we didn't want to get
> > in the business of patching third party libraries to allow SIGSYS.
>
> My approach is slightly different here. While I agree that pr_info() is
> useful to find which syscalls are not whitelisted yet, it doesn't solve
> the different issue. That is - when qemu calls a not whitelisted
> syscall, currently it gets killed without any error being printed
> anywhere. So libvirt just sees the machine has gone away and it can be
> really hard to figure out the root cause is actually sandboxing.

Have you looked at the audit_seccomp() call in the kernel's 
__secure_computing() function?  It may already be doing what you're 
looking for.

> Re patch itself. Okay, there may be some situations where signal is not
> delivered or missed in which case we don't print any error message. But
> in these situations qemu should check / is checking return value of a
> syscall so it will die itself anyway.

This just sounds like it is defeating the purpose of seccomp, and a lot 
of things could go wrong with signal handling.  The entire point of 
SCMP_ACT_KILL is that the process does not get control back after an 
non-whitelisted syscall is called.  Now we'd be giving back control to 
what could be a malicious guest.

> Michal

--
Regards,
Corey Bryant