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

[Michal Privoznik]

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.

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.

Michal