Re: [PATCH v3 2/5] seccomp: add the concept of a seccomp filter FD

[Tycho Andersen]

On Wed, Sep 30, 2015 at 11:27:34AM -0700, Andy Lutomirski wrote:
> On Wed, Sep 30, 2015 at 11:13 AM, Tycho Andersen
>  wrote:
> > This patch introduces the concept of a seccomp fd, with a similar interface
> > and usage to ebpf fds. Initially, one is allowed to create, install, and
> > dump these fds. Any manipulation of seccomp fds requires users to be root
> > in their own user namespace, matching the checks done for
> > SECCOMP_SET_MODE_FILTER.
> >
> > Installing a filterfd has some gotchas, though. Andy mentioned previously
> > that we should restrict installation to filter fds whose parent is already
> > in the filter tree. This doesn't quite work in the case of created seccomp
> > fds, since once you install a filter fd, you can't install any other filter
> > fd since it has no parent and there is no way to "pre-chain" filters before
> > installing them.
> 
> ISTM, if we like the seccomp fd approach, we should have them be
> created with a parent already set.  IOW the default should be that
> their parent is the creator's seccomp fd and, if needed, creators
> could specify a different parent.

Allowing people doing SECCOMP_FD_NEW to specify a parent fd would
work. Then we can disallow installing a seccomp fd if its parent is
not the current filter, and get rid of the whole mess with prev
locking and all that.

Tycho
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] seccomp: add the concept of a seccomp filter FD

[Andy Lutomirski]

On Wed, Sep 30, 2015 at 11:36 AM, Tycho Andersen
 wrote:
> On Wed, Sep 30, 2015 at 11:27:34AM -0700, Andy Lutomirski wrote:
>> On Wed, Sep 30, 2015 at 11:13 AM, Tycho Andersen
>>  wrote:
>> > This patch introduces the concept of a seccomp fd, with a similar interface
>> > and usage to ebpf fds. Initially, one is allowed to create, install, and
>> > dump these fds. Any manipulation of seccomp fds requires users to be root
>> > in their own user namespace, matching the checks done for
>> > SECCOMP_SET_MODE_FILTER.
>> >
>> > Installing a filterfd has some gotchas, though. Andy mentioned previously
>> > that we should restrict installation to filter fds whose parent is already
>> > in the filter tree. This doesn't quite work in the case of created seccomp
>> > fds, since once you install a filter fd, you can't install any other filter
>> > fd since it has no parent and there is no way to "pre-chain" filters before
>> > installing them.
>>
>> ISTM, if we like the seccomp fd approach, we should have them be
>> created with a parent already set.  IOW the default should be that
>> their parent is the creator's seccomp fd and, if needed, creators
>> could specify a different parent.
>
> Allowing people doing SECCOMP_FD_NEW to specify a parent fd would
> work. Then we can disallow installing a seccomp fd if its parent is
> not the current filter, and get rid of the whole mess with prev
> locking and all that.
>

Yes, please.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/5] seccomp: add the concept of a seccomp filter FD

[Tycho Andersen]

This patch introduces the concept of a seccomp fd, with a similar interface
and usage to ebpf fds. Initially, one is allowed to create, install, and
dump these fds. Any manipulation of seccomp fds requires users to be root
in their own user namespace, matching the checks done for
SECCOMP_SET_MODE_FILTER.

Installing a filterfd has some gotchas, though. Andy mentioned previously
that we should restrict installation to filter fds whose parent is already
in the filter tree. This doesn't quite work in the case of created seccomp
fds, since once you install a filter fd, you can't install any other filter
fd since it has no parent and there is no way to "pre-chain" filters before
installing them. To work around this, we allow installing filters who have
no parent. If the filter has a parent, we require the current filter try to
be an ancestor of it.

I'm not quite sure that the ancestor restriction is correct, since it can
still allow for "re-parenting" of filters, potentially introducing new
filters to a task. However, since these operations are limited to root in
the user ns, perhaps it is ok. There is also some potentially racy
behavior where a task re-parents a filter that another task has installed.

One option to work around this is to keep a bit on struct seccomp_filter to
allow each filter to have its parent set exactly once. (This would still
allow you to install a filter multiple times, as long as the parent was the
same in each case.)

Signed-off-by: Tycho Andersen 
CC: Kees Cook 
CC: Will Drewry 
CC: Oleg Nesterov 
CC: Andy Lutomirski 
CC: Pavel Emelyanov 
CC: Serge E. Hallyn 
CC: Alexei Starovoitov 
CC: Daniel Borkmann 
---
 include/uapi/linux/seccomp.h |  24 ++
 kernel/seccomp.c | 189 ++-
 2 files changed, 210 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 0f238a4..4ee8770 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -13,10 +13,16 @@
 /* Valid operations for seccomp syscall. */
 #define SECCOMP_SET_MODE_STRICT0
 #define SECCOMP_SET_MODE_FILTER1
+#define SECCOMP_FILTER_FD  2
 
 /* Valid flags for SECCOMP_SET_MODE_FILTER */
 #define SECCOMP_FILTER_FLAG_TSYNC  1
 
+/* Valid commands for SECCOMP_FILTER_FD */
+#define SECCOMP_FD_NEW 0
+#define SECCOMP_FD_INSTALL 1
+#define SECCOMP_FD_DUMP2
+
 /*
  * All BPF programs must return a 32-bit value.
  * The bottom 16-bits are for optional return data.
@@ -51,4 +57,22 @@ struct seccomp_data {
__u64 args[6];
 };
 
+struct seccomp_fd {
+   __u32   size;
+
+   union {
+   /* SECCOMP_FD_NEW */
+   struct sock_fprog __user*new_prog;
+
+   /* SECCOMP_FD_INSTALL */
+   int install_fd;
+
+   /* SECCOMP_FD_DUMP */
+   struct {
+   int dump_fd;
+   struct sock_filter __user   *insns;
+   };
+   };
+};
+
 #endif /* _UAPI_LINUX_SECCOMP_H */
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 09f3769..6f0465c 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -26,6 +26,8 @@
 #endif
 
 #ifdef CONFIG_SECCOMP_FILTER
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -58,6 +60,7 @@ struct seccomp_filter {
atomic_t usage;
struct seccomp_filter *prev;
struct bpf_prog *prog;
+   spinlock_t prev_lock;
 };
 
 /* Limit any path through the tree to 256KB worth of instructions. */
@@ -393,6 +396,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct 
sock_fprog *fprog)
}
 
atomic_set(>usage, 1);
+   sfilter->prev_lock = __SPIN_LOCK_UNLOCKED(>prev_lock);
 
return sfilter;
 }
@@ -441,6 +445,7 @@ static long seccomp_attach_filter(unsigned int flags,
struct seccomp_filter *walker;
 
assert_spin_locked(>sighand->siglock);
+   assert_spin_locked(>prev_lock);
 
/* Validate resulting filter length. */
total_insns = filter->prog->len;
@@ -482,10 +487,8 @@ void get_seccomp_filter(struct task_struct *tsk)
atomic_inc(>usage);
 }
 
-/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
-void put_seccomp_filter(struct task_struct *tsk)
+static void seccomp_filter_decref(struct seccomp_filter *orig)
 {
-   struct seccomp_filter *orig = tsk->seccomp.filter;
/* Clean up single-reference branches iteratively. */
while (orig && atomic_dec_and_test(>usage)) {
struct seccomp_filter *freeme = orig;
@@ -494,6 +497,12 @@ void put_seccomp_filter(struct task_struct *tsk)
}
 }
 
+/* put_seccomp_filter - decrements the ref count of 

Re: [PATCH v3 2/5] seccomp: add the concept of a seccomp filter FD

[Andy Lutomirski]

On Wed, Sep 30, 2015 at 11:13 AM, Tycho Andersen
 wrote:
> This patch introduces the concept of a seccomp fd, with a similar interface
> and usage to ebpf fds. Initially, one is allowed to create, install, and
> dump these fds. Any manipulation of seccomp fds requires users to be root
> in their own user namespace, matching the checks done for
> SECCOMP_SET_MODE_FILTER.
>
> Installing a filterfd has some gotchas, though. Andy mentioned previously
> that we should restrict installation to filter fds whose parent is already
> in the filter tree. This doesn't quite work in the case of created seccomp
> fds, since once you install a filter fd, you can't install any other filter
> fd since it has no parent and there is no way to "pre-chain" filters before
> installing them.

ISTM, if we like the seccomp fd approach, we should have them be
created with a parent already set.  IOW the default should be that
their parent is the creator's seccomp fd and, if needed, creators
could specify a different parent.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] seccomp: add the concept of a seccomp filter FD

[kbuild test robot]

Hi Tycho,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please 
ignore]

config: i386-alldefconfig (attached as .config)
reproduce:
  git checkout 9613ae6bf5f111701614acb3eda3123d21a59239
  # save the attached .config to linux build tree
  make ARCH=i386 

All error/warnings (new ones prefixed by >>):

>> kernel/seccomp.c:998:10: error: expected ';', ',' or ')' before 'const'
 const char __user *filter)
 ^
   kernel/seccomp.c: In function 'do_seccomp':
>> kernel/seccomp.c:1016:10: error: implicit declaration of function 
>> 'seccomp_filter_fd' [-Werror=implicit-function-declaration]
  return seccomp_filter_fd(flags, uargs);
 ^
   cc1: some warnings being treated as errors

vim +998 kernel/seccomp.c

   992 const char __user *filter)
   993  {
   994  return -EINVAL;
   995  }
   996  
   997  static inline long seccomp_filter_fd(unsigned int flags
 > 998   const char __user *filter)
   999  {
  1000  return -EINVAL;
  1001  }
  1002  #endif
  1003  
  1004  /* Common entry point for both prctl and syscall. */
  1005  static long do_seccomp(unsigned int op, unsigned int flags,
  1006 const char __user *uargs)
  1007  {
  1008  switch (op) {
  1009  case SECCOMP_SET_MODE_STRICT:
  1010  if (flags != 0 || uargs != NULL)
  1011  return -EINVAL;
  1012  return seccomp_set_mode_strict();
  1013  case SECCOMP_SET_MODE_FILTER:
  1014  return seccomp_set_mode_filter(flags, uargs);
  1015  case SECCOMP_FILTER_FD:
> 1016  return seccomp_filter_fd(flags, uargs);
  1017  default:
  1018  return -EINVAL;
  1019  }

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data