[Qemu-devel] [PULL 00/05] seccomp branch queue

[Qemu-devel] [PULL 00/05] seccomp branch queue

[Eduardo Otubo]

The following changes since commit c49d3411faae8ffaab8f7e5db47405a008411c10:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-10-12' into staging (2015-10-13 10:42:06 +0100)

are available in the git repository at:

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

for you to fetch changes up to b1e1f0bbe7268d0bb8f63da220b41803b2e54081:

  seccomp: loosen library version dependency (2015-10-30 14:33:00 +0100)

----------------------------------------------------------------
seccomp branch queue

----------------------------------------------------------------
Andrew Jones (2):
      seccomp: add cacheflush to whitelist
      configure: arm/aarch64: allow enable-seccomp

Namsun Ch'o (2):
      seccomp: add madvise, shmget, and shmctl to whitelist
      seccomp: add setuid, setgid, chroot and setgroups to whitelist

dann frazier (1):
      seccomp: loosen library version dependency

 configure      | 32 ++++++++++++++++++++++-------
 qemu-seccomp.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 86 insertions(+), 11 deletions(-)

-- 
2.1.4

[Qemu-devel] [PULL 01/05] seccomp: add cacheflush to whitelist

[Eduardo Otubo]

From: Andrew Jones <drjones@redhat.com>

cacheflush is an arm-specific syscall that qemu built for arm
uses. Add it to the whitelist.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Acked-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
---
 qemu-seccomp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index f9de0d3..33644a4 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -237,7 +237,8 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(fadvise64), 240 },
     { SCMP_SYS(inotify_init1), 240 },
     { SCMP_SYS(inotify_add_watch), 240 },
-    { SCMP_SYS(mbind), 240 }
+    { SCMP_SYS(mbind), 240 },
+    { SCMP_SYS(cacheflush), 240 },
 };
 
 int seccomp_start(void)
-- 
2.1.4

[Qemu-devel] [PULL 02/05] configure: arm/aarch64: allow enable-seccomp

[Eduardo Otubo]

From: Andrew Jones <drjones@redhat.com>

This is a revert of ae6e8ef11e6cb, but with a bit of refactoring,
and also specifically adding arm/aarch64, rather than all
architectures. Currently, libseccomp code appears to also support
mips, ppc, and s390. We could therefore allow qemu to enable
seccomp for those platforms as well, with additional configure
patches, given they're tested and proven to work.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Acked-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
---
 configure | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/configure b/configure
index f08327e..7d5aab2 100755
--- a/configure
+++ b/configure
@@ -1876,16 +1876,34 @@ fi
 # libseccomp check
 
 if test "$seccomp" != "no" ; then
-    if test "$cpu" = "i386" || test "$cpu" = "x86_64" &&
-        $pkg_config --atleast-version=2.1.1 libseccomp; then
+    case "$cpu" in
+    i386|x86_64)
+        libseccomp_minver="2.1.1"
+        ;;
+    arm|aarch64)
+        libseccomp_minver="2.2.3"
+        ;;
+    *)
+        libseccomp_minver=""
+        ;;
+    esac
+
+    if test "$libseccomp_minver" != "" &&
+       $pkg_config --atleast-version=$libseccomp_minver libseccomp ; then
         libs_softmmu="$libs_softmmu `$pkg_config --libs libseccomp`"
         QEMU_CFLAGS="$QEMU_CFLAGS `$pkg_config --cflags libseccomp`"
-	seccomp="yes"
+        seccomp="yes"
     else
-	if test "$seccomp" = "yes"; then
-            feature_not_found "libseccomp" "Install libseccomp devel >= 2.1.1"
-	fi
-	seccomp="no"
+        if test "$seccomp" = "yes" ; then
+            if test "$libseccomp_minver" != "" ; then
+                feature_not_found "libseccomp" \
+                    "Install libseccomp devel >= $libseccomp_minver"
+            else
+                feature_not_found "libseccomp" \
+                    "libseccomp is not supported for host cpu $cpu"
+            fi
+        fi
+        seccomp="no"
     fi
 fi
 ##########################################
-- 
2.1.4

[Qemu-devel] [PULL 03/05] seccomp: add madvise, shmget, and shmctl to whitelist

[Eduardo Otubo]

From: Namsun Ch'o <namnamc@Safe-mail.net>

This patch introduces madvise, shmget, shmctl and its flags to the
seccomp whitelist. This prevents Qemu to break in case of using -runas
or chroot with sandbox enabled.

Signed-off-by: Namsun Ch'o <namnamc@safe-mail.net>
Acked-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
---
Changelog:
v1
 - Created argument filters for the madvise, shmget, and shmctl syscalls.
v1 -> v2
 - Added 5 new madvise flags which were present in the source code but not in
   the strace which I generated.
 - Added IP_CREAT|0600 to shmget, which Daniel Berrange pointed out was
   present in GTK2, which QEMU uses but does not call directly.
v2 -> v3
 - Replaced include asm/mman-common.h with sys/mman.h which is more proper.
 - Fixed typo where I had IP_CREAT instead of IPC_CREAT.
 - Removed the comma on the last entry of the madvise_flags array.
 - Removed one madvise flag (MADV_INVALID) which doesn't exist, apparently.

 qemu-seccomp.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 33644a4..e7a54e8 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -14,6 +14,8 @@
  */
 #include <stdio.h>
 #include <seccomp.h>
+#include <linux/ipc.h>
+#include <sys/mman.h>
 #include "sysemu/seccomp.h"
 
 struct QemuSeccompSyscall {
@@ -105,7 +107,6 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(rt_sigreturn), 245 },
     { SCMP_SYS(sync), 245 },
     { SCMP_SYS(pread64), 245 },
-    { SCMP_SYS(madvise), 245 },
     { SCMP_SYS(set_robust_list), 245 },
     { SCMP_SYS(lseek), 245 },
     { SCMP_SYS(pselect6), 245 },
@@ -224,11 +225,9 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(arch_prctl), 240 },
     { SCMP_SYS(mkdir), 240 },
     { SCMP_SYS(fchmod), 240 },
-    { SCMP_SYS(shmget), 240 },
     { SCMP_SYS(shmat), 240 },
     { SCMP_SYS(shmdt), 240 },
     { SCMP_SYS(timerfd_create), 240 },
-    { SCMP_SYS(shmctl), 240 },
     { SCMP_SYS(mlockall), 240 },
     { SCMP_SYS(mlock), 240 },
     { SCMP_SYS(munlock), 240 },
@@ -265,6 +264,59 @@ int seccomp_start(void)
         }
     }
 
+    /* madvise */
+    static const int madvise_flags[] = {
+        MADV_DODUMP,
+        MADV_DONTDUMP,
+        MADV_UNMERGEABLE,
+        MADV_WILLNEED,
+        MADV_DONTFORK,
+        MADV_DONTNEED,
+        MADV_HUGEPAGE,
+        MADV_MERGEABLE
+    };
+    for (i = 0; i < ARRAY_SIZE(madvise_flags); i++) {
+        rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(madvise), 1,
+            SCMP_A2(SCMP_CMP_EQ, madvise_flags[i]));
+        if (rc < 0) {
+            goto seccomp_return;
+        }
+    }
+    rc = seccomp_syscall_priority(ctx, SCMP_SYS(madvise), 245);
+    if (rc < 0) {
+        goto seccomp_return;
+    }
+
+    /* shmget */
+    rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(shmget), 2,
+        SCMP_A0(SCMP_CMP_EQ, IPC_PRIVATE),
+        SCMP_A2(SCMP_CMP_EQ, IPC_CREAT|0777));
+    if (rc < 0) {
+        goto seccomp_return;
+    }
+    rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(shmget), 2,
+        SCMP_A0(SCMP_CMP_EQ, IPC_PRIVATE),
+        SCMP_A2(SCMP_CMP_EQ, IPC_CREAT|0600));
+    if (rc < 0) {
+        goto seccomp_return;
+    }
+    rc = seccomp_syscall_priority(ctx, SCMP_SYS(shmget), 240);
+    if (rc < 0) {
+        goto seccomp_return;
+    }
+
+    /* shmctl */
+    rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(shmctl), 2,
+        SCMP_A1(SCMP_CMP_EQ, IPC_RMID),
+        SCMP_A2(SCMP_CMP_EQ, 0));
+    if (rc < 0) {
+        goto seccomp_return;
+    }
+    rc = seccomp_syscall_priority(ctx, SCMP_SYS(shmctl), 240);
+    if (rc < 0) {
+        goto seccomp_return;
+    }
+
     rc = seccomp_load(ctx);
 
   seccomp_return:
-- 
2.1.4

[Qemu-devel] [PULL 04/05] seccomp: add setuid, setgid, chroot and setgroups to whitelist

[Eduardo Otubo]

From: Namsun Ch'o <namnamc@Safe-mail.net>

The seccomp sandbox doesn't whitelist setuid, setgid, or setgroups, which are
needed for -runas to work. It also doesn't whitelist chroot, which is needed
for the -chroot option. Unfortunately, QEMU enables seccomp before it drops
privileges or chroots, so without these whitelisted, -runas and -chroot cause
QEMU to be killed with -sandbox on. This patch adds those syscalls.

Signed-off-by: Namsun Ch'o <namnamc@safe-mail.net>
Acked-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
---
 qemu-seccomp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index e7a54e8..877fd88 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -238,6 +238,10 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(inotify_add_watch), 240 },
     { SCMP_SYS(mbind), 240 },
     { SCMP_SYS(cacheflush), 240 },
+    { SCMP_SYS(setuid), 240 },
+    { SCMP_SYS(setgid), 240 },
+    { SCMP_SYS(chroot), 240 },
+    { SCMP_SYS(setgroups), 240 },
 };
 
 int seccomp_start(void)
-- 
2.1.4

[Qemu-devel] [PULL 05/05] seccomp: loosen library version dependency

[Eduardo Otubo]

From: dann frazier <dann.frazier@canonical.com>

Drop the libseccomp required version back to 2.1.0, restoring the ability
to build w/ --enable-seccomp on Ubuntu 14.04.

Commit 4cc47f8b3cc4f32586ba2f7fce1dc267da774a69 tightened the dependency
on libseccomp from version 2.1.0 to 2.1.1. This broke building on Ubuntu
14.04, the current Ubuntu LTS release. The commit message didn't mention
any specific functional need for 2.1.1, just that it was the most recent
stable version at the time. I reviewed the changes between 2.1.0 and 2.1.1,
but it looks like that update just contained minor fixes and cleanups - no
obvious (to me) new interfaces or critical bug fixes.

Signed-off-by: dann frazier <dann.frazier@canonical.com>
Acked-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 7d5aab2..8a9794b 100755
--- a/configure
+++ b/configure
@@ -1878,7 +1878,7 @@ fi
 if test "$seccomp" != "no" ; then
     case "$cpu" in
     i386|x86_64)
-        libseccomp_minver="2.1.1"
+        libseccomp_minver="2.1.0"
         ;;
     arm|aarch64)
         libseccomp_minver="2.2.3"
-- 
2.1.4

Re: [Qemu-devel] [PULL 00/05] seccomp branch queue

[Peter Maydell]

On 30 October 2015 at 13:44, Eduardo Otubo
<eduardo.otubo@profitbricks.com> wrote:
> The following changes since commit c49d3411faae8ffaab8f7e5db47405a008411c10:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-10-12' into staging (2015-10-13 10:42:06 +0100)
>
> are available in the git repository at:
>
>   git://github.com/otubo/qemu.git tags/pull-seccomp-20151030
>
> for you to fetch changes up to b1e1f0bbe7268d0bb8f63da220b41803b2e54081:
>
>   seccomp: loosen library version dependency (2015-10-30 14:33:00 +0100)
>
> ----------------------------------------------------------------
> seccomp branch queue
>
> ----------------------------------------------------------------

Hi. I'm afraid this fails to build on x86 Linux:

/home/petmay01/linaro/qemu-for-merges/qemu-seccomp.c:241:8: error:
‘__NR_cacheflush’ undeclared here (not in a function)
     { SCMP_SYS(cacheflush), 240 },
        ^

Ubuntu Trusty with these versions of libseccomp packages:

ii  libseccomp-dev:amd64                                  2.1.0+dfsg-1
                                       amd64        high level
interface to Linux seccomp filter (development files)
ii  libseccomp2:amd64                                     2.1.0+dfsg-1
                                       amd64        high level
interface to Linux seccomp filter

(It also has a trivial conflict with master, which you can
deal with if you rebase.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/05] seccomp branch queue

[Andrew Jones]

On Fri, Oct 30, 2015 at 04:30:05PM +0000, Peter Maydell wrote:
> On 30 October 2015 at 13:44, Eduardo Otubo
> <eduardo.otubo@profitbricks.com> wrote:
> > The following changes since commit c49d3411faae8ffaab8f7e5db47405a008411c10:
> >
> >   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-10-12' into staging (2015-10-13 10:42:06 +0100)
> >
> > are available in the git repository at:
> >
> >   git://github.com/otubo/qemu.git tags/pull-seccomp-20151030
> >
> > for you to fetch changes up to b1e1f0bbe7268d0bb8f63da220b41803b2e54081:
> >
> >   seccomp: loosen library version dependency (2015-10-30 14:33:00 +0100)
> >
> > ----------------------------------------------------------------
> > seccomp branch queue
> >
> > ----------------------------------------------------------------
> 
> Hi. I'm afraid this fails to build on x86 Linux:
> 
> /home/petmay01/linaro/qemu-for-merges/qemu-seccomp.c:241:8: error:
> ‘__NR_cacheflush’ undeclared here (not in a function)
>      { SCMP_SYS(cacheflush), 240 },

Ugh...  Of course we can't have a common seccomp syscall table and different
atleast-versions for different architectures... So x86 only requires 2.1.0,
but libseccomp didn't know about cacheflush until 2.2.1, and we require 2.2.3
for other reasons. Ideally, we'd compose the seccomp syscall table by starting
with a common base (something 2.1.0 knows about) and then add an architecture
specific table, and maybe even a machine-type specific table. That's pretty
easy to do, we just need a list of tables, but as it's a framework change and
will touch many places, then I don't expect it to be a super quick job... I
think in the interim the fix for patch 1/5 of this series is something
like shown below. If there are no objections, then I'll send it.

drew

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 80d034a8d5190..e76097e958779 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -16,6 +16,10 @@
 #include <seccomp.h>
 #include "sysemu/seccomp.h"
 
+#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
+#define HAVE_CACHEFLUSH
+#endif
+
 struct QemuSeccompSyscall {
     int32_t num;
     uint8_t priority;
@@ -238,7 +242,10 @@ static const struct QemuSeccompSyscall
seccomp_whitelist[] = {
     { SCMP_SYS(inotify_init1), 240 },
     { SCMP_SYS(inotify_add_watch), 240 },
-    { SCMP_SYS(mbind), 240 }
+    { SCMP_SYS(mbind), 240 },
+#ifdef HAVE_CACHEFLUSH
+    { SCMP_SYS(cacheflush), 240 },
+#endif
 };
 
 int seccomp_start(void)

Re: [Qemu-devel] [PULL 04/05] seccomp: add setuid, setgid, chroot and setgroups to whitelist

[Paolo Bonzini]

On 30/10/2015 14:44, Eduardo Otubo wrote:
> From: Namsun Ch'o <namnamc@Safe-mail.net>
> 
> The seccomp sandbox doesn't whitelist setuid, setgid, or setgroups, which are
> needed for -runas to work. It also doesn't whitelist chroot, which is needed
> for the -chroot option. Unfortunately, QEMU enables seccomp before it drops
> privileges or chroots, so without these whitelisted, -runas and -chroot cause
> QEMU to be killed with -sandbox on. This patch adds those syscalls.

I think this patch should not be applied, because it completely defeats
the purpose of the sandbox.  With these syscalls whitelisted, -runas and
-chroot have absolutely no effect against an attacker, even with
-sandbox on.

Paolo

[Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Andrew Jones]

cacheflush is an arm-specific syscall that qemu built for arm
uses. Add it to the whitelist, but only if we're linking with
a recent enough libseccomp.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
v2: only add cacheflush if libseccomp supports it

 qemu-seccomp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 80d034a8d5190..e76097e958779 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -16,6 +16,10 @@
 #include <seccomp.h>
 #include "sysemu/seccomp.h"
 
+#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
+#define HAVE_CACHEFLUSH
+#endif
+
 struct QemuSeccompSyscall {
     int32_t num;
     uint8_t priority;
@@ -238,7 +242,10 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(inotify_init1), 240 },
     { SCMP_SYS(inotify_add_watch), 240 },
     { SCMP_SYS(mbind), 240 },
-    { SCMP_SYS(memfd_create), 240 }
+    { SCMP_SYS(memfd_create), 240 },
+#ifdef HAVE_CACHEFLUSH
+    { SCMP_SYS(cacheflush), 240 },
+#endif
 };
 
 int seccomp_start(void)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Peter Maydell]

On 2 November 2015 at 17:56, Andrew Jones <drjones@redhat.com> wrote:
> cacheflush is an arm-specific syscall that qemu built for arm
> uses. Add it to the whitelist, but only if we're linking with
> a recent enough libseccomp.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> v2: only add cacheflush if libseccomp supports it
>
>  qemu-seccomp.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 80d034a8d5190..e76097e958779 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -16,6 +16,10 @@
>  #include <seccomp.h>
>  #include "sysemu/seccomp.h"
>
> +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
> +#define HAVE_CACHEFLUSH
> +#endif

This will claim that a hypothetical future version 3.0.0 does not
have cacheflush...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Andrew Jones]

On Mon, Nov 02, 2015 at 06:09:41PM +0000, Peter Maydell wrote:
> On 2 November 2015 at 17:56, Andrew Jones <drjones@redhat.com> wrote:
> > cacheflush is an arm-specific syscall that qemu built for arm
> > uses. Add it to the whitelist, but only if we're linking with
> > a recent enough libseccomp.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > v2: only add cacheflush if libseccomp supports it
> >
> >  qemu-seccomp.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > index 80d034a8d5190..e76097e958779 100644
> > --- a/qemu-seccomp.c
> > +++ b/qemu-seccomp.c
> > @@ -16,6 +16,10 @@
> >  #include <seccomp.h>
> >  #include "sysemu/seccomp.h"
> >
> > +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
> > +#define HAVE_CACHEFLUSH
> > +#endif
> 
> This will claim that a hypothetical future version 3.0.0 does not
> have cacheflush...

Indeed. Sigh... In that case, how about just

#if defined(TARGET_ARM) || defined(TARGET_AARCH64)
    { SCMP_SYS(cacheflush), 240 },
#endif

Thanks,
drew

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Peter Maydell]

On 2 November 2015 at 19:04, Andrew Jones <drjones@redhat.com> wrote:
> On Mon, Nov 02, 2015 at 06:09:41PM +0000, Peter Maydell wrote:
>> On 2 November 2015 at 17:56, Andrew Jones <drjones@redhat.com> wrote:
>> > cacheflush is an arm-specific syscall that qemu built for arm
>> > uses. Add it to the whitelist, but only if we're linking with
>> > a recent enough libseccomp.
>> >
>> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>> > ---
>> > v2: only add cacheflush if libseccomp supports it
>> >
>> >  qemu-seccomp.c | 9 ++++++++-
>> >  1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
>> > index 80d034a8d5190..e76097e958779 100644
>> > --- a/qemu-seccomp.c
>> > +++ b/qemu-seccomp.c
>> > @@ -16,6 +16,10 @@
>> >  #include <seccomp.h>
>> >  #include "sysemu/seccomp.h"
>> >
>> > +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
>> > +#define HAVE_CACHEFLUSH
>> > +#endif
>>
>> This will claim that a hypothetical future version 3.0.0 does not
>> have cacheflush...
>
> Indeed. Sigh... In that case, how about just
>
> #if defined(TARGET_ARM) || defined(TARGET_AARCH64)
>     { SCMP_SYS(cacheflush), 240 },
> #endif

You want to be checking based on the host architecture,
not the target architecture. Also, not doing the check based
on seccomp version means there's no hint in the code that the
ifdefs become obsolete if we raise our cross-architecture
minimum seccomp version requirement in the future, so really
a version check is better I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] seccomp: add cacheflush to whitelist

[Andrew Jones]

On Mon, Nov 02, 2015 at 08:37:15PM +0000, Peter Maydell wrote:
> On 2 November 2015 at 19:04, Andrew Jones <drjones@redhat.com> wrote:
> > On Mon, Nov 02, 2015 at 06:09:41PM +0000, Peter Maydell wrote:
> >> On 2 November 2015 at 17:56, Andrew Jones <drjones@redhat.com> wrote:
> >> > cacheflush is an arm-specific syscall that qemu built for arm
> >> > uses. Add it to the whitelist, but only if we're linking with
> >> > a recent enough libseccomp.
> >> >
> >> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> > ---
> >> > v2: only add cacheflush if libseccomp supports it
> >> >
> >> >  qemu-seccomp.c | 9 ++++++++-
> >> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> >> > index 80d034a8d5190..e76097e958779 100644
> >> > --- a/qemu-seccomp.c
> >> > +++ b/qemu-seccomp.c
> >> > @@ -16,6 +16,10 @@
> >> >  #include <seccomp.h>
> >> >  #include "sysemu/seccomp.h"
> >> >
> >> > +#if SCMP_VER_MAJOR >= 2 && SCMP_VER_MINOR >= 2 && SCMP_VER_MICRO >= 3
> >> > +#define HAVE_CACHEFLUSH
> >> > +#endif
> >>
> >> This will claim that a hypothetical future version 3.0.0 does not
> >> have cacheflush...
> >
> > Indeed. Sigh... In that case, how about just
> >
> > #if defined(TARGET_ARM) || defined(TARGET_AARCH64)
> >     { SCMP_SYS(cacheflush), 240 },
> > #endif
> 
> You want to be checking based on the host architecture,
> not the target architecture. Also, not doing the check based
> on seccomp version means there's no hint in the code that the
> ifdefs become obsolete if we raise our cross-architecture
> minimum seccomp version requirement in the future, so really
> a version check is better I think.

Right. OK, I'll stop flinging junk and pull a better version
check together.

Thanks,
drew

> 
> thanks
> -- PMM
> 

[Qemu-devel] [PATCH v3] seccomp: add cacheflush to whitelist

[Andrew Jones]

cacheflush is an arm-specific syscall that qemu built for arm
uses. Add it to the whitelist, but only if we're linking with
a recent enough libseccomp.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
v3: deal with major and minor version number bumps
v2: only add cacheflush if libseccomp supports it

 qemu-seccomp.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 80d034a8d5190..c831fe83ad500 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -16,6 +16,14 @@
 #include <seccomp.h>
 #include "sysemu/seccomp.h"
 
+#if SCMP_VER_MAJOR >= 3
+  #define HAVE_CACHEFLUSH
+#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR >= 3
+  #define HAVE_CACHEFLUSH
+#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR == 2 && SCMP_VER_MICRO >= 3
+  #define HAVE_CACHEFLUSH
+#endif
+
 struct QemuSeccompSyscall {
     int32_t num;
     uint8_t priority;
@@ -238,7 +246,10 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(inotify_init1), 240 },
     { SCMP_SYS(inotify_add_watch), 240 },
     { SCMP_SYS(mbind), 240 },
-    { SCMP_SYS(memfd_create), 240 }
+    { SCMP_SYS(memfd_create), 240 },
+#ifdef HAVE_CACHEFLUSH
+    { SCMP_SYS(cacheflush), 240 },
+#endif
 };
 
 int seccomp_start(void)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3] seccomp: add cacheflush to whitelist

[Andrew Jones]

On Mon, Nov 02, 2015 at 11:53:26PM +0100, Andrew Jones wrote:
> cacheflush is an arm-specific syscall that qemu built for arm
> uses. Add it to the whitelist, but only if we're linking with
> a recent enough libseccomp.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> v3: deal with major and minor version number bumps
> v2: only add cacheflush if libseccomp supports it
> 
>  qemu-seccomp.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 80d034a8d5190..c831fe83ad500 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -16,6 +16,14 @@
>  #include <seccomp.h>
>  #include "sysemu/seccomp.h"
>  
> +#if SCMP_VER_MAJOR >= 3
> +  #define HAVE_CACHEFLUSH
> +#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR >= 3
> +  #define HAVE_CACHEFLUSH
> +#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR == 2 && SCMP_VER_MICRO >= 3
> +  #define HAVE_CACHEFLUSH
> +#endif
> +
>  struct QemuSeccompSyscall {
>      int32_t num;
>      uint8_t priority;
> @@ -238,7 +246,10 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
>      { SCMP_SYS(inotify_init1), 240 },
>      { SCMP_SYS(inotify_add_watch), 240 },
>      { SCMP_SYS(mbind), 240 },
> -    { SCMP_SYS(memfd_create), 240 }
> +    { SCMP_SYS(memfd_create), 240 },
> +#ifdef HAVE_CACHEFLUSH
> +    { SCMP_SYS(cacheflush), 240 },
> +#endif
>  };
>  
>  int seccomp_start(void)
> -- 
> 1.8.3.1
>

Eduardo, ping?

drew 

Re: [Qemu-devel] [PATCH v3] seccomp: add cacheflush to whitelist

[Eduardo Otubo]

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

On Mon, Nov 09, 2015 at 04=47=53PM -0500, Andrew Jones wrote:
> On Mon, Nov 02, 2015 at 11:53:26PM +0100, Andrew Jones wrote:
> > cacheflush is an arm-specific syscall that qemu built for arm
> > uses. Add it to the whitelist, but only if we're linking with
> > a recent enough libseccomp.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > v3: deal with major and minor version number bumps
> > v2: only add cacheflush if libseccomp supports it
> > 
> >  qemu-seccomp.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > index 80d034a8d5190..c831fe83ad500 100644
> > --- a/qemu-seccomp.c
> > +++ b/qemu-seccomp.c
> > @@ -16,6 +16,14 @@
> >  #include <seccomp.h>
> >  #include "sysemu/seccomp.h"
> >  
> > +#if SCMP_VER_MAJOR >= 3
> > +  #define HAVE_CACHEFLUSH
> > +#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR >= 3
> > +  #define HAVE_CACHEFLUSH
> > +#elif SCMP_VER_MAJOR == 2 && SCMP_VER_MINOR == 2 && SCMP_VER_MICRO >= 3
> > +  #define HAVE_CACHEFLUSH
> > +#endif
> > +
> >  struct QemuSeccompSyscall {
> >      int32_t num;
> >      uint8_t priority;
> > @@ -238,7 +246,10 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
> >      { SCMP_SYS(inotify_init1), 240 },
> >      { SCMP_SYS(inotify_add_watch), 240 },
> >      { SCMP_SYS(mbind), 240 },
> > -    { SCMP_SYS(memfd_create), 240 }
> > +    { SCMP_SYS(memfd_create), 240 },
> > +#ifdef HAVE_CACHEFLUSH
> > +    { SCMP_SYS(cacheflush), 240 },
> > +#endif

Acked-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>

> >  };
> >  
> >  int seccomp_start(void)
> > -- 
> > 1.8.3.1
> >
> 
> Eduardo, ping?

Thanks for the ping. I was busy and forgot about these patches.
Nothing special to be said, acked.

I'll send a pull request as soon as possible.

-- 
Eduardo Otubo
ProfitBricks GmbH

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PULL 04/05] seccomp: add setuid, setgid, chroot and setgroups to whitelist

[Eduardo Otubo]

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

On Mon, Nov 02, 2015 at 08=51=26AM +0100, Paolo Bonzini wrote:
> 
> 
> On 30/10/2015 14:44, Eduardo Otubo wrote:
> > From: Namsun Ch'o <namnamc@Safe-mail.net>
> > 
> > The seccomp sandbox doesn't whitelist setuid, setgid, or setgroups, which are
> > needed for -runas to work. It also doesn't whitelist chroot, which is needed
> > for the -chroot option. Unfortunately, QEMU enables seccomp before it drops
> > privileges or chroots, so without these whitelisted, -runas and -chroot cause
> > QEMU to be killed with -sandbox on. This patch adds those syscalls.
> 
> I think this patch should not be applied, because it completely defeats
> the purpose of the sandbox.  With these syscalls whitelisted, -runas and
> -chroot have absolutely no effect against an attacker, even with
> -sandbox on.
> 

Also, Namsun's emails are bouncing back. Don't know if it's worth to
merge them with no valid author's contact.

-- 
Eduardo Otubo
ProfitBricks GmbH

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]