Re: [Qemu-devel] [PATCH v3 3/3] seccomp: set the seccomp filter to all threads

[Marc-André Lureau]

On Wed, Aug 22, 2018 at 5:46 PM, Daniel P. Berrangé <address@hidden> wrote:
> On Wed, Aug 22, 2018 at 04:29:56PM +0200, Marc-André Lureau wrote:
>> When using "-seccomp on", the seccomp policy is only applied to the
>> main thread, the vcpu worker thread and other worker threads created
>> after seccomp policy is applied; the seccomp policy is not applied to
>> e.g. the RCU thread because it is created before the seccomp policy is
>> applied and SECCOMP_FILTER_FLAG_TSYNC isn't used.
>>
>> This can be verified with
>> for task in /proc/`pidof qemu`/task/*; do cat $task/status | grep Secc ; done
>> Seccomp:      2
>> Seccomp:      0
>> Seccomp:      0
>> Seccomp:      2
>> Seccomp:      2
>> Seccomp:      2
>>
>> Starting with libseccomp 2.2.0 and kernel >= 3.17, we can use
>> seccomp_attr_set(ctx, > SCMP_FLTATR_CTL_TSYNC, 1) to update the policy
>> on all threads.
>>
>> Do it by default if possible, warn if not possible. Add an option to
>> set the tsync behaviour explicitly.
>>
>> Note: we can't bump libseccomp to 2.2.0 since it's not available in
>> Debian oldstable (2.1.0).
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  qemu-seccomp.c  | 65 +++++++++++++++++++++++++++++++++++++++++++++++--
>>  qemu-options.hx |  2 ++
>>  2 files changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
>> index f0c833f3ca..aa23eae970 100644
>> --- a/qemu-seccomp.c
>> +++ b/qemu-seccomp.c
>> @@ -119,6 +119,45 @@ qemu_seccomp(unsigned int operation, unsigned int 
>> flags, void *args)
>>  #endif
>>  }
>>
>> +static bool qemu_seccomp_syscall_check(void)
>> +{
>> +    int rc;
>> +
>> +    /*
>> +     * this is an invalid call because the second argument is non-zero, but
>> +     * depending on the errno value of ENOSYS or EINVAL we can guess if the
>> +     * seccomp() syscal is supported or not
>> +     */
>> +    rc = qemu_seccomp(SECCOMP_SET_MODE_STRICT, 1, NULL);
>> +    if (rc < 0 && errno == EINVAL) {
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static bool qemu_seccomp_get_default_tsync(void)
>> +{
>> +    bool tsync = true;
>> +
>> +    /* TSYNC support was added with the syscall */
>> +    if (!qemu_seccomp_syscall_check()) {
>> +        error_report("The host kernel doesn't support seccomp TSYNC!");
>> +        tsync = false;
>> +    }
>> +
>> +#if !(SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2)
>> +    error_report("libseccomp is too old to support TSYNC!");
>> +    tsync = false;
>> +#endif
>> +
>> +    if (!tsync) {
>> +        error_report("Only the main thread will be filtered by seccomp!");
>
> At this point you might as well not bother using seccomp at all. The
> thread that is confined merely needs to scribble something into the
> stack of the unconfined thread and now it can do whatever it wants.

Actually, that message is incorrect, it should rather be "not all
threads will be filtered" (as described in commit message).

> IMHO we need to find a way to get the policy to apply to those other
> threads.

That's what the patch is about ;)

> The RCU thread is tricky as it is spawned from a __constructor__
> function, which means it'll be active way before we setup seccomp.
>
> I think we need to figure out a way todo synchronization between
> the RCU thread and the seccomp setup code. Could we have a global
> variable 'int seccomp_initialized' that we check from the RCU
> thread loop - when that toggles to non-zero, the RCU thread can
> then call into the seccomp_start() method to activate policy in
> its thread. We'd need a synchronous feedback mechansim back to
> the main thread, as it must block startup until all the threads
> have activated the seccomp filter.

That's a bit like TSYNC, except we do it ourself with RCU thread. But
what about other threads? For examples one that could be created by
external libraries (like mesa)

>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 5515dfaba5..dafacb60c6 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3864,6 +3864,8 @@ Disable set*uid|gid system calls
>>  Disable *fork and execve
>>  @item address@hidden
>>  Disable process affinity and schedular priority
>> address@hidden address@hidden
>> +Apply seccomp filter to all threads (default is auto, and will warn if fail)
>
> IMHO this should never exist, as setting "tsync" to anything other
> than "yes", is akin to just running without any sandbox.

Then we should just fail -sandbox on those systems.

>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|