[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]

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

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

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

[Eduardo Otubo]

Hi,

This is the seccomp branch queue with fixes regarding a build crach on armv7l
and adding new syscalls to the whitelist. Details below.

The following changes since commit 6e76d125f244e10676b917208f2a074729820246:

  Update version for v2.2.0-rc0 release (2014-11-05 15:21:04 +0000)

are available in the git repository at:

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

for you to fetch changes up to 4cc47f8b3cc4f32586ba2f7fce1dc267da774a69:

  seccomp: change configure to avoid arm 32 to break (2014-11-11 17:05:21 +0100)

----------------------------------------------------------------
seccomp: change configure to avoid arm 32 to break
seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()
----------------------------------------------------------------
Eduardo Otubo (1):
      seccomp: change configure to avoid arm 32 to break

Philipp Gesang (1):
      seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()

 configure      | 3 ++-
 qemu-seccomp.c | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Eduardo Otubo (1):
  seccomp: change configure to avoid arm 32 to break

Philipp Gesang (1):
  seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1()
    and inotify_add_watch()

 configure      | 3 ++-
 qemu-seccomp.c | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
1.9.1

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

[Peter Maydell]

On 11 November 2014 16:21, Eduardo Otubo <eduardo.otubo@profitbricks.com> wrote:
> Hi,
>
> This is the seccomp branch queue with fixes regarding a build crach on armv7l
> and adding new syscalls to the whitelist. Details below.
>
> The following changes since commit 6e76d125f244e10676b917208f2a074729820246:
>
>   Update version for v2.2.0-rc0 release (2014-11-05 15:21:04 +0000)
>
> are available in the git repository at:
>
>   git://github.com/otubo/qemu.git tags/pull-seccomp-20141111
>
> for you to fetch changes up to 4cc47f8b3cc4f32586ba2f7fce1dc267da774a69:
>
>   seccomp: change configure to avoid arm 32 to break (2014-11-11 17:05:21 +0100)
>
> ----------------------------------------------------------------
> seccomp: change configure to avoid arm 32 to break
> seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

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

[Eduardo Otubo]

Hi,

This is the seccomp branch queue with fixes regarding a build crach on armv7l
and adding new syscalls to the whitelist. Details below.

The following changes since commit 6e76d125f244e10676b917208f2a074729820246:

  Update version for v2.2.0-rc0 release (2014-11-05 15:21:04 +0000)

are available in the git repository at:

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

for you to fetch changes up to f5c76d3eb66a63604d9d8b47ce94268f9babea10:

  seccomp: change configure to avoid arm 32 to break (2014-11-07 16:42:18 +0100)

----------------------------------------------------------------
seccomp: change configure to avoid arm 32 to break
seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()

----------------------------------------------------------------
Eduardo Otubo (1):
      seccomp: change configure to avoid arm 32 to break

Philipp Gesang (1):
      seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()

 configure      | 3 ++-
 qemu-seccomp.c | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
1.9.1

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

[Peter Maydell]

On 10 November 2014 14:21, Eduardo Otubo <eduardo.otubo@profitbricks.com> wrote:
> Hi,
>
> This is the seccomp branch queue with fixes regarding a build crach on armv7l
> and adding new syscalls to the whitelist. Details below.
>
> The following changes since commit 6e76d125f244e10676b917208f2a074729820246:
>
>   Update version for v2.2.0-rc0 release (2014-11-05 15:21:04 +0000)
>
> are available in the git repository at:
>
>   git://github.com/otubo/qemu.git tags/pull-seccomp-20141110
>
> for you to fetch changes up to f5c76d3eb66a63604d9d8b47ce94268f9babea10:
>
>   seccomp: change configure to avoid arm 32 to break (2014-11-07 16:42:18 +0100)
>
> ----------------------------------------------------------------
> seccomp: change configure to avoid arm 32 to break
> seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()
>
> ----------------------------------------------------------------
> Eduardo Otubo (1):
>       seccomp: change configure to avoid arm 32 to break
>
> Philipp Gesang (1):
>       seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()

Hi. I'm afraid I'm going to have to ask you to respin this,
because the patches are missing various reviewed-by/signed-off etc
tags.

Firstly, and least critically, the configure patch should have
the reviewed-by and acked-by tags that people have posted on
the mailing list for it; it's useful to keep a permanent record
of these in the commit history (for instance it can give a good
idea of who to cc if a change turns out to have a bug in future).

More significantly, the patch from Philipp is missing a
Signed-off-by: line from you. Every commit in a pullreq should
have a signed-off-by: from the submaintainer(s) of the tree
that's being pulled, because it represents your assertion that
these changes are good to go into QEMU without further checking.

Sorry to have to be picky about this. The good news is, your
signed tag seems OK. You should make sure you've pushed your
public key out to the public keyservers if you haven't already
(gpg --send-keys 12F8BD2F) since it's not currently visible
there [there is a lag of a day or two though so if you've
sent it out recently it's probably just in that delay.]

thanks
-- PMM

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

[Eduardo Otubo]

On Mon, Nov 10, 2014 at 08:50:07PM +0000, Peter Maydell wrote:
> On 10 November 2014 14:21, Eduardo Otubo <eduardo.otubo@profitbricks.com> wrote:
> > Hi,
> >
> > This is the seccomp branch queue with fixes regarding a build crach on armv7l
> > and adding new syscalls to the whitelist. Details below.
> >
> > The following changes since commit 6e76d125f244e10676b917208f2a074729820246:
> >
> >   Update version for v2.2.0-rc0 release (2014-11-05 15:21:04 +0000)
> >
> > are available in the git repository at:
> >
> >   git://github.com/otubo/qemu.git tags/pull-seccomp-20141110
> >
> > for you to fetch changes up to f5c76d3eb66a63604d9d8b47ce94268f9babea10:
> >
> >   seccomp: change configure to avoid arm 32 to break (2014-11-07 16:42:18 +0100)
> >
> > ----------------------------------------------------------------
> > seccomp: change configure to avoid arm 32 to break
> > seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()
> >
> > ----------------------------------------------------------------
> > Eduardo Otubo (1):
> >       seccomp: change configure to avoid arm 32 to break
> >
> > Philipp Gesang (1):
> >       seccomp: whitelist syscalls fallocate(), fadvise64(), inotify_init1() and inotify_add_watch()
> 
> Hi. I'm afraid I'm going to have to ask you to respin this,
> because the patches are missing various reviewed-by/signed-off etc
> tags.
> 
> Firstly, and least critically, the configure patch should have
> the reviewed-by and acked-by tags that people have posted on
> the mailing list for it; it's useful to keep a permanent record
> of these in the commit history (for instance it can give a good
> idea of who to cc if a change turns out to have a bug in future).
> 
> More significantly, the patch from Philipp is missing a
> Signed-off-by: line from you. Every commit in a pullreq should
> have a signed-off-by: from the submaintainer(s) of the tree
> that's being pulled, because it represents your assertion that
> these changes are good to go into QEMU without further checking.
> 

My apologies! I was so worried about the key and tagging being set
properly on my branch that those lines completely passed from my review.
I resent the pull request with everything double-checked. I hope
everything is 0K now.

> Sorry to have to be picky about this. The good news is, your
> signed tag seems OK. You should make sure you've pushed your
> public key out to the public keyservers if you haven't already
> (gpg --send-keys 12F8BD2F) since it's not currently visible
> there [there is a lag of a day or two though so if you've
> sent it out recently it's probably just in that delay.]
> 

I just pushed my key to the public server. So it might take a while to
appear as valid.

Thanks for the help on this,
and sorry again for the lapses.

Regards,

-- 
Eduardo Otubo
ProfitBricks GmbH