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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

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 <otubo@linux.vnet.ibm.com>
>>> ---
>>>   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