[Qemu-devel] [PULL 0/2] seccomp branch queue

[Qemu-devel] [PULL 0/2] seccomp branch queue

[Eduardo Otubo]

The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:

  Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)

are available in the Git repository at:

  https://github.com/otubo/qemu.git tags/pull-seccomp-20180725

for you to fetch changes up to 5b2f59307372bae13a2ff95706646674eccb65e0:

  RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available (2018-07-25 16:07:31 +0200)

----------------------------------------------------------------
pull-seccomp-20180725

----------------------------------------------------------------
Marc-André Lureau (2):
      seccomp: use SIGSYS signal instead of killing the thread
      RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available

 qemu-seccomp.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

-- 
2.17.1

[Qemu-devel] [PULL 1/2] seccomp: use SIGSYS signal instead of killing the thread

[Eduardo Otubo]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The seccomp action SCMP_ACT_KILL results in immediate termination of
the thread that made the bad system call. However, qemu being
multi-threaded, it keeps running. There is no easy way for parent
process / management layer (libvirt) to know about that situation.

Instead, the default SIGSYS handler when invoked with SCMP_ACT_TRAP
will terminate the program and core dump.

This may not be the most secure solution, but probably better than
just killing the offending thread. SCMP_ACT_KILL_PROCESS has been
added in Linux 4.14 to improve the situation, which I propose to use
by default if available in the next patch.

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1594456

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Eduardo Otubo <otubo@redhat.com>
---
 qemu-seccomp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 9cd8eb9499..b117a92559 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -125,7 +125,7 @@ static int seccomp_start(uint32_t seccomp_opts)
             continue;
         }
 
-        rc = seccomp_rule_add_array(ctx, SCMP_ACT_KILL, blacklist[i].num,
+        rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
                                     blacklist[i].narg, blacklist[i].arg_cmp);
         if (rc < 0) {
             goto seccomp_return;
-- 
2.17.1

[Qemu-devel] [PULL 2/2] RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available

[Eduardo Otubo]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The upcoming libseccomp release should have SCMP_ACT_KILL_PROCESS
action (https://github.com/seccomp/libseccomp/issues/96).

SCMP_ACT_KILL_PROCESS is preferable to immediately terminate the
offending process, rather than having the SIGSYS handler running.

Use SECCOMP_GET_ACTION_AVAIL to check availability of kernel support,
as libseccomp will fallback on SCMP_ACT_KILL otherwise, and we still
prefer SCMP_ACT_TRAP.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Eduardo Otubo <otubo@redhat.com>
---
 qemu-seccomp.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index b117a92559..505887d5af 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -20,6 +20,7 @@
 #include <sys/prctl.h>
 #include <seccomp.h>
 #include "sysemu/seccomp.h"
+#include <linux/seccomp.h>
 
 /* For some architectures (notably ARM) cacheflush is not supported until
  * libseccomp 2.2.3, but configure enforces that we are using a more recent
@@ -107,12 +108,39 @@ static const struct QemuSeccompSyscall blacklist[] = {
     { SCMP_SYS(sched_get_priority_min), QEMU_SECCOMP_SET_RESOURCECTL },
 };
 
+static inline int
+qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
+{
+#ifdef __NR_seccomp
+    return syscall(__NR_seccomp, operation, flags, args);
+#else
+    return -1;
+#endif
+}
+
+static uint32_t qemu_seccomp_get_kill_action(void)
+{
+#if defined(SECCOMP_GET_ACTION_AVAIL) && defined(SCMP_ACT_KILL_PROCESS) && \
+    defined(SECCOMP_RET_KILL_PROCESS)
+    {
+        uint32_t action = SECCOMP_RET_KILL_PROCESS;
+
+        if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0) {
+            return SCMP_ACT_KILL_PROCESS;
+        }
+    }
+#endif
+
+    return SCMP_ACT_TRAP;
+}
+
 
 static int seccomp_start(uint32_t seccomp_opts)
 {
     int rc = 0;
     unsigned int i = 0;
     scmp_filter_ctx ctx;
+    uint32_t action = qemu_seccomp_get_kill_action();
 
     ctx = seccomp_init(SCMP_ACT_ALLOW);
     if (ctx == NULL) {
@@ -125,7 +153,7 @@ static int seccomp_start(uint32_t seccomp_opts)
             continue;
         }
 
-        rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
+        rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
                                     blacklist[i].narg, blacklist[i].arg_cmp);
         if (rc < 0) {
             goto seccomp_return;
-- 
2.17.1

Re: [Qemu-devel] [PULL 0/2] seccomp branch queue

[Peter Maydell]

On 25 July 2018 at 15:16, Eduardo Otubo <otubo@redhat.com> wrote:
> The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:
>
>   Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/otubo/qemu.git tags/pull-seccomp-20180725
>
> for you to fetch changes up to 5b2f59307372bae13a2ff95706646674eccb65e0:
>
>   RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available (2018-07-25 16:07:31 +0200)
>
> ----------------------------------------------------------------
> pull-seccomp-20180725
>
> ----------------------------------------------------------------
> Marc-André Lureau (2):
>       seccomp: use SIGSYS signal instead of killing the thread
>       RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available

Hi. This fails to compile with clang:

  CC      qemu-seccomp.o
qemu-seccomp.c:112:1: error: unused function 'qemu_seccomp'
[-Werror,-Wunused-function]
qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
^

This is because clang is stricter about warning about static inline
functions defined in .c files but never used and your ifdef
guard on the callsite is not matched by one around the function
definition.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/2] seccomp branch queue

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On 26/07/2018 - 11:47:46, Peter Maydell wrote:
> On 25 July 2018 at 15:16, Eduardo Otubo <otubo@redhat.com> wrote:
> > The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:
> >
> >   Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)
> >
> > are available in the Git repository at:
> >
> >   https://github.com/otubo/qemu.git tags/pull-seccomp-20180725
> >
> > for you to fetch changes up to 5b2f59307372bae13a2ff95706646674eccb65e0:
> >
> >   RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available (2018-07-25 16:07:31 +0200)
> >
> > ----------------------------------------------------------------
> > pull-seccomp-20180725
> >
> > ----------------------------------------------------------------
> > Marc-André Lureau (2):
> >       seccomp: use SIGSYS signal instead of killing the thread
> >       RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available
> 
> Hi. This fails to compile with clang:
> 
>   CC      qemu-seccomp.o
> qemu-seccomp.c:112:1: error: unused function 'qemu_seccomp'
> [-Werror,-Wunused-function]
> qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
> ^
> 
> This is because clang is stricter about warning about static inline
> functions defined in .c files but never used and your ifdef
> guard on the callsite is not matched by one around the function
> definition.
> 

Peter, sorry for not catching that before.
Marc, can you fix and resend?

-- 
Eduardo Otubo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PULL 0/2] seccomp branch queue

[Marc-André Lureau]

Hi

On Thu, Jul 26, 2018 at 12:47 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 25 July 2018 at 15:16, Eduardo Otubo <otubo@redhat.com> wrote:
>> The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:
>>
>>   Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)
>>
>> are available in the Git repository at:
>>
>>   https://github.com/otubo/qemu.git tags/pull-seccomp-20180725
>>
>> for you to fetch changes up to 5b2f59307372bae13a2ff95706646674eccb65e0:
>>
>>   RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available (2018-07-25 16:07:31 +0200)
>>
>> ----------------------------------------------------------------
>> pull-seccomp-20180725
>>
>> ----------------------------------------------------------------
>> Marc-André Lureau (2):
>>       seccomp: use SIGSYS signal instead of killing the thread
>>       RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available
>
> Hi. This fails to compile with clang:
>
>   CC      qemu-seccomp.o
> qemu-seccomp.c:112:1: error: unused function 'qemu_seccomp'
> [-Werror,-Wunused-function]
> qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
> ^
>
> This is because clang is stricter about warning about static inline
> functions defined in .c files but never used and your ifdef
> guard on the callsite is not matched by one around the function
> definition.
>

https://lkml.org/lkml/2017/6/6/631 ;)



-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL 0/2] seccomp branch queue

[Marc-André Lureau]

H

On Thu, Jul 26, 2018 at 2:04 PM, Eduardo Otubo <otubo@redhat.com> wrote:
> On 26/07/2018 - 11:47:46, Peter Maydell wrote:
>> On 25 July 2018 at 15:16, Eduardo Otubo <otubo@redhat.com> wrote:
>> > The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:
>> >
>> >   Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)
>> >
>> > are available in the Git repository at:
>> >
>> >   https://github.com/otubo/qemu.git tags/pull-seccomp-20180725
>> >
>> > for you to fetch changes up to 5b2f59307372bae13a2ff95706646674eccb65e0:
>> >
>> >   RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available (2018-07-25 16:07:31 +0200)
>> >
>> > ----------------------------------------------------------------
>> > pull-seccomp-20180725
>> >
>> > ----------------------------------------------------------------
>> > Marc-André Lureau (2):
>> >       seccomp: use SIGSYS signal instead of killing the thread
>> >       RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available
>>
>> Hi. This fails to compile with clang:
>>
>>   CC      qemu-seccomp.o
>> qemu-seccomp.c:112:1: error: unused function 'qemu_seccomp'
>> [-Werror,-Wunused-function]
>> qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
>> ^
>>
>> This is because clang is stricter about warning about static inline
>> functions defined in .c files but never used and your ifdef
>> guard on the callsite is not matched by one around the function
>> definition.
>>
>
> Peter, sorry for not catching that before.
> Marc, can you fix and resend?

I suggest to drop that patch from 3.0. Since it will require a newer
libseccomp to be actually useful, it can be delayed imho.



-- 
Marc-André Lureau