[PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Nicolas Schichan]

Architecture must select HAVE_SECCOMP_FILTER_JIT and implement
seccomp_jit_compile() and seccomp_jit_free() if they intend to support
jitted seccomp filters.

struct seccomp_filter has been moved to <linux/seccomp.h> to make its
content available to the jit compilation code.

In a way similar to the net BPF, the jit compilation code is expected
to updates struct seccomp_filter.bpf_func pointer to the generated
code.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/Kconfig            |   14 ++++++++++++++
 include/linux/seccomp.h |   39 +++++++++++++++++++++++++++++++++++++++
 kernel/seccomp.c        |   34 +++++-----------------------------
 3 files changed, 58 insertions(+), 29 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 5a1779c..1284367 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -339,6 +339,10 @@ config HAVE_ARCH_SECCOMP_FILTER
 	  - secure_computing return value is checked and a return value of -1
 	    results in the system call being skipped immediately.
 
+# Used by archs to tell that they support SECCOMP_FILTER_JIT
+config HAVE_SECCOMP_FILTER_JIT
+	bool
+
 config SECCOMP_FILTER
 	def_bool y
 	depends on HAVE_ARCH_SECCOMP_FILTER && SECCOMP && NET
@@ -349,6 +353,16 @@ config SECCOMP_FILTER
 
 	  See Documentation/prctl/seccomp_filter.txt for details.
 
+config SECCOMP_FILTER_JIT
+	bool "enable Seccomp filter Just In Time compiler"
+	depends on HAVE_SECCOMP_FILTER_JIT && BPF_JIT && SECCOMP_FILTER
+	help
+	  Seccomp syscall filtering capabilities are normally handled
+	  by an interpreter. This option allows kernel to generate a native
+	  code when filter is loaded in memory. This should speedup
+	  syscall filtering. Note : Admin should enable this feature
+	  changing /proc/sys/net/core/bpf_jit_enable
+
 config HAVE_CONTEXT_TRACKING
 	bool
 	help
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 6f19cfd..af27494 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -6,6 +6,7 @@
 #ifdef CONFIG_SECCOMP
 
 #include <linux/thread_info.h>
+#include <linux/filter.h>
 #include <asm/seccomp.h>
 
 struct seccomp_filter;
@@ -47,6 +48,44 @@ static inline int seccomp_mode(struct seccomp *s)
 	return s->mode;
 }
 
+/**
+ * struct seccomp_filter - container for seccomp BPF programs
+ *
+ * @usage: reference count to manage the object lifetime.
+ *         get/put helpers should be used when accessing an instance
+ *         outside of a lifetime-guarded section.  In general, this
+ *         is only needed for handling filters shared across tasks.
+ * @prev: points to a previously installed, or inherited, filter
+ * @len: the number of instructions in the program
+ * @insns: the BPF program instructions to evaluate
+ *
+ * seccomp_filter objects are organized in a tree linked via the @prev
+ * pointer.  For any task, it appears to be a singly-linked list starting
+ * with current->seccomp.filter, the most recently attached or inherited filter.
+ * However, multiple filters may share a @prev node, by way of fork(), which
+ * results in a unidirectional tree existing in memory.  This is similar to
+ * how namespaces work.
+ *
+ * seccomp_filter objects should never be modified after being attached
+ * to a task_struct (other than @usage).
+ */
+struct seccomp_filter {
+	atomic_t usage;
+	struct seccomp_filter *prev;
+	unsigned short len;  /* Instruction count */
+	unsigned int (*bpf_func)(const struct sk_buff *skb,
+				 const struct sock_filter *filter);
+	struct sock_filter insns[];
+};
+
+#ifdef CONFIG_SECCOMP_FILTER_JIT
+extern void seccomp_jit_compile(struct seccomp_filter *fp);
+extern void seccomp_jit_free(struct seccomp_filter *fp);
+#else
+static inline void seccomp_jit_compile(struct seccomp_filter *fp) { }
+static inline void seccomp_jit_free(struct seccomp_filter *fp) { }
+#endif
+
 #else /* CONFIG_SECCOMP */
 
 #include <linux/errno.h>
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index b7a1004..a1aadaa 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -30,34 +30,6 @@
 #include <linux/tracehook.h>
 #include <linux/uaccess.h>
 
-/**
- * struct seccomp_filter - container for seccomp BPF programs
- *
- * @usage: reference count to manage the object lifetime.
- *         get/put helpers should be used when accessing an instance
- *         outside of a lifetime-guarded section.  In general, this
- *         is only needed for handling filters shared across tasks.
- * @prev: points to a previously installed, or inherited, filter
- * @len: the number of instructions in the program
- * @insns: the BPF program instructions to evaluate
- *
- * seccomp_filter objects are organized in a tree linked via the @prev
- * pointer.  For any task, it appears to be a singly-linked list starting
- * with current->seccomp.filter, the most recently attached or inherited filter.
- * However, multiple filters may share a @prev node, by way of fork(), which
- * results in a unidirectional tree existing in memory.  This is similar to
- * how namespaces work.
- *
- * seccomp_filter objects should never be modified after being attached
- * to a task_struct (other than @usage).
- */
-struct seccomp_filter {
-	atomic_t usage;
-	struct seccomp_filter *prev;
-	unsigned short len;  /* Instruction count */
-	struct sock_filter insns[];
-};
-
 /* Limit any path through the tree to 256KB worth of instructions. */
 #define MAX_INSNS_PER_PATH ((1 << 18) / sizeof(struct sock_filter))
 
@@ -213,7 +185,7 @@ static u32 seccomp_run_filters(int syscall)
 	 * value always takes priority (ignoring the DATA).
 	 */
 	for (f = current->seccomp.filter; f; f = f->prev) {
-		u32 cur_ret = sk_run_filter(NULL, f->insns);
+		u32 cur_ret = f->bpf_func(NULL, f->insns);
 		if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
 			ret = cur_ret;
 	}
@@ -275,6 +247,9 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
 	if (ret)
 		goto fail;
 
+	filter->bpf_func = sk_run_filter;
+	seccomp_jit_compile(filter);
+
 	/*
 	 * If there is an existing filter, make it the prev and don't drop its
 	 * task reference.
@@ -332,6 +307,7 @@ void put_seccomp_filter(struct task_struct *tsk)
 	while (orig && atomic_dec_and_test(&orig->usage)) {
 		struct seccomp_filter *freeme = orig;
 		orig = orig->prev;
+		seccomp_jit_free(freeme);
 		kfree(freeme);
 	}
 }
-- 
1.7.10.4

Re: [PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Nicolas Schichan]

On 03/15/2013 07:28 PM, Nicolas Schichan wrote:
[Sorry, I forgot to put the mailing lists as the receivers of the introductory 
message]

Hi,

This patch serie adds support for jitted seccomp BPF filters, with the
required modifications to make it work on the ARM architecture.

- The first patch in the serie adds the required boiler plate in the
   core kernel seccomp code to invoke the JIT compilation/free code.

- The second patch reworks the ARM BPF JIT code to make the generation
   process less dependent on struct sk_filter.

- The last patch actually implements the ARM part in the BPF jit code.

Some benchmarks, on a 1.6Ghz 88f6282 CPU:

Each system call is tested in two way (fast/slow):

  - on the fast version, the tested system call is accepted immediately
    after checking the architecture (5 BPF instructions).

  - on the slow version, the tested system call is accepted after
    previously checking for 85 syscall (90 instructions, including the
    architecture check).

The tested syscall is invoked in a loop 1000000 time, the reported
time is the time spent in the loop in seconds.

Without Seccomp JIT:

Syscall		Time-Fast  Time-Slow
--------------- ---------- ----------
gettimeofday	0.389	   1.633
getpid		0.406	   1.688
getresuid	1.003	   2.266
getcwd		1.342	   2.128

With Seccomp JIT:

Syscall		Time-Fast  Time-Slow
--------------- ----------- ---------
gettimeofday	0.348	    0.428
getpid		0.365	    0.480
getresuid	0.981	    1.060
getcwd		1.237	    1.294

For reference, the same code without any seccomp filter:

Syscall		Time
--------------- -----
gettimeofday	0.119
getpid		0.137
getresuid	0.747
getcwd		1.021

The activation of the BPF JIT for seccomp is still controled with the
/proc/sys/net/core/bpf_jit_enable sysctl knob.

Those changes are based on the latest rmk-for-next branch.

Regards,

-- 
Nicolas Schichan
Freebox SAS

[PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Nicolas Schichan]

On 03/15/2013 07:28 PM, Nicolas Schichan wrote:
[Sorry, I forgot to put the mailing lists as the receivers of the introductory 
message]

Hi,

This patch serie adds support for jitted seccomp BPF filters, with the
required modifications to make it work on the ARM architecture.

- The first patch in the serie adds the required boiler plate in the
   core kernel seccomp code to invoke the JIT compilation/free code.

- The second patch reworks the ARM BPF JIT code to make the generation
   process less dependent on struct sk_filter.

- The last patch actually implements the ARM part in the BPF jit code.

Some benchmarks, on a 1.6Ghz 88f6282 CPU:

Each system call is tested in two way (fast/slow):

  - on the fast version, the tested system call is accepted immediately
    after checking the architecture (5 BPF instructions).

  - on the slow version, the tested system call is accepted after
    previously checking for 85 syscall (90 instructions, including the
    architecture check).

The tested syscall is invoked in a loop 1000000 time, the reported
time is the time spent in the loop in seconds.

Without Seccomp JIT:

Syscall		Time-Fast  Time-Slow
--------------- ---------- ----------
gettimeofday	0.389	   1.633
getpid		0.406	   1.688
getresuid	1.003	   2.266
getcwd		1.342	   2.128

With Seccomp JIT:

Syscall		Time-Fast  Time-Slow
--------------- ----------- ---------
gettimeofday	0.348	    0.428
getpid		0.365	    0.480
getresuid	0.981	    1.060
getcwd		1.237	    1.294

For reference, the same code without any seccomp filter:

Syscall		Time
--------------- -----
gettimeofday	0.119
getpid		0.137
getresuid	0.747
getcwd		1.021

The activation of the BPF JIT for seccomp is still controled with the
/proc/sys/net/core/bpf_jit_enable sysctl knob.

Those changes are based on the latest rmk-for-next branch.

Regards,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Kees Cook]

On Fri, Mar 15, 2013 at 11:28 AM, Nicolas Schichan <nschichan@freebox.fr> wrote:
> Architecture must select HAVE_SECCOMP_FILTER_JIT and implement
> seccomp_jit_compile() and seccomp_jit_free() if they intend to support
> jitted seccomp filters.
>
> struct seccomp_filter has been moved to <linux/seccomp.h> to make its
> content available to the jit compilation code.
>
> In a way similar to the net BPF, the jit compilation code is expected
> to updates struct seccomp_filter.bpf_func pointer to the generated
> code.

Exciting. :) Thanks for working on this!

>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> ---
>  arch/Kconfig            |   14 ++++++++++++++
>  include/linux/seccomp.h |   39 +++++++++++++++++++++++++++++++++++++++
>  kernel/seccomp.c        |   34 +++++-----------------------------
>  3 files changed, 58 insertions(+), 29 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 5a1779c..1284367 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -339,6 +339,10 @@ config HAVE_ARCH_SECCOMP_FILTER
>           - secure_computing return value is checked and a return value of -1
>             results in the system call being skipped immediately.
>
> +# Used by archs to tell that they support SECCOMP_FILTER_JIT
> +config HAVE_SECCOMP_FILTER_JIT
> +       bool
> +
>  config SECCOMP_FILTER
>         def_bool y
>         depends on HAVE_ARCH_SECCOMP_FILTER && SECCOMP && NET
> @@ -349,6 +353,16 @@ config SECCOMP_FILTER
>
>           See Documentation/prctl/seccomp_filter.txt for details.
>
> +config SECCOMP_FILTER_JIT
> +       bool "enable Seccomp filter Just In Time compiler"
> +       depends on HAVE_SECCOMP_FILTER_JIT && BPF_JIT && SECCOMP_FILTER
> +       help
> +         Seccomp syscall filtering capabilities are normally handled
> +         by an interpreter. This option allows kernel to generate a native
> +         code when filter is loaded in memory. This should speedup
> +         syscall filtering. Note : Admin should enable this feature
> +         changing /proc/sys/net/core/bpf_jit_enable
> +
>  config HAVE_CONTEXT_TRACKING
>         bool
>         help
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index 6f19cfd..af27494 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -6,6 +6,7 @@
>  #ifdef CONFIG_SECCOMP
>
>  #include <linux/thread_info.h>
> +#include <linux/filter.h>
>  #include <asm/seccomp.h>
>
>  struct seccomp_filter;
> @@ -47,6 +48,44 @@ static inline int seccomp_mode(struct seccomp *s)
>         return s->mode;
>  }
>
> +/**
> + * struct seccomp_filter - container for seccomp BPF programs
> + *
> + * @usage: reference count to manage the object lifetime.
> + *         get/put helpers should be used when accessing an instance
> + *         outside of a lifetime-guarded section.  In general, this
> + *         is only needed for handling filters shared across tasks.
> + * @prev: points to a previously installed, or inherited, filter
> + * @len: the number of instructions in the program
> + * @insns: the BPF program instructions to evaluate

This should be updated to include the new bpf_func field.

Regardless, it'd be better to not expose this structure to userspace.

> + *
> + * seccomp_filter objects are organized in a tree linked via the @prev
> + * pointer.  For any task, it appears to be a singly-linked list starting
> + * with current->seccomp.filter, the most recently attached or inherited filter.
> + * However, multiple filters may share a @prev node, by way of fork(), which
> + * results in a unidirectional tree existing in memory.  This is similar to
> + * how namespaces work.
> + *
> + * seccomp_filter objects should never be modified after being attached
> + * to a task_struct (other than @usage).
> + */
> +struct seccomp_filter {
> +       atomic_t usage;
> +       struct seccomp_filter *prev;
> +       unsigned short len;  /* Instruction count */
> +       unsigned int (*bpf_func)(const struct sk_buff *skb,
> +                                const struct sock_filter *filter);
> +       struct sock_filter insns[];
> +};
> +
> +#ifdef CONFIG_SECCOMP_FILTER_JIT
> +extern void seccomp_jit_compile(struct seccomp_filter *fp);
> +extern void seccomp_jit_free(struct seccomp_filter *fp);
> +#else
> +static inline void seccomp_jit_compile(struct seccomp_filter *fp) { }
> +static inline void seccomp_jit_free(struct seccomp_filter *fp) { }
> +#endif
> +
>  #else /* CONFIG_SECCOMP */
>
>  #include <linux/errno.h>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index b7a1004..a1aadaa 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -30,34 +30,6 @@
>  #include <linux/tracehook.h>
>  #include <linux/uaccess.h>
>
> -/**
> - * struct seccomp_filter - container for seccomp BPF programs
> - *
> - * @usage: reference count to manage the object lifetime.
> - *         get/put helpers should be used when accessing an instance
> - *         outside of a lifetime-guarded section.  In general, this
> - *         is only needed for handling filters shared across tasks.
> - * @prev: points to a previously installed, or inherited, filter
> - * @len: the number of instructions in the program
> - * @insns: the BPF program instructions to evaluate
> - *
> - * seccomp_filter objects are organized in a tree linked via the @prev
> - * pointer.  For any task, it appears to be a singly-linked list starting
> - * with current->seccomp.filter, the most recently attached or inherited filter.
> - * However, multiple filters may share a @prev node, by way of fork(), which
> - * results in a unidirectional tree existing in memory.  This is similar to
> - * how namespaces work.
> - *
> - * seccomp_filter objects should never be modified after being attached
> - * to a task_struct (other than @usage).
> - */
> -struct seccomp_filter {
> -       atomic_t usage;
> -       struct seccomp_filter *prev;
> -       unsigned short len;  /* Instruction count */
> -       struct sock_filter insns[];
> -};
> -
>  /* Limit any path through the tree to 256KB worth of instructions. */
>  #define MAX_INSNS_PER_PATH ((1 << 18) / sizeof(struct sock_filter))
>
> @@ -213,7 +185,7 @@ static u32 seccomp_run_filters(int syscall)
>          * value always takes priority (ignoring the DATA).
>          */
>         for (f = current->seccomp.filter; f; f = f->prev) {
> -               u32 cur_ret = sk_run_filter(NULL, f->insns);
> +               u32 cur_ret = f->bpf_func(NULL, f->insns);

This will make bpf_func a rather attractive target inside the kernel.
I wonder if there could be ways to harden it against potential attack.

>                 if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
>                         ret = cur_ret;
>         }
> @@ -275,6 +247,9 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
>         if (ret)
>                 goto fail;
>
> +       filter->bpf_func = sk_run_filter;
> +       seccomp_jit_compile(filter);
> +
>         /*
>          * If there is an existing filter, make it the prev and don't drop its
>          * task reference.
> @@ -332,6 +307,7 @@ void put_seccomp_filter(struct task_struct *tsk)
>         while (orig && atomic_dec_and_test(&orig->usage)) {
>                 struct seccomp_filter *freeme = orig;
>                 orig = orig->prev;
> +               seccomp_jit_free(freeme);
>                 kfree(freeme);
>         }
>  }
> --
> 1.7.10.4
>

In addition to this work, I'm curious if anyone has looked at JIT
hardening, to make it a less trivial ROP target? For example:
http://grsecurity.net/~spender/jit_prot.diff

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Nicolas Schichan]

On 03/15/2013 07:45 PM, Kees Cook wrote:
> On Fri, Mar 15, 2013 at 11:28 AM, Nicolas Schichan <nschichan@freebox.fr> wrote:
>> +/**
>> + * struct seccomp_filter - container for seccomp BPF programs
>> + *
>> + * @usage: reference count to manage the object lifetime.
>> + *         get/put helpers should be used when accessing an instance
>> + *         outside of a lifetime-guarded section.  In general, this
>> + *         is only needed for handling filters shared across tasks.
>> + * @prev: points to a previously installed, or inherited, filter
>> + * @len: the number of instructions in the program
>> + * @insns: the BPF program instructions to evaluate
>
> This should be updated to include the new bpf_func field.

Sure, I'll fix this in a re-spin of the patch serie.

> Regardless, it'd be better to not expose this structure to userspace.

Yes, I did not realise that this header was exported to userspace. Do you know 
any place not exported to userspace where the structure definition would be 
appropriate ?

>> @@ -213,7 +185,7 @@ static u32 seccomp_run_filters(int syscall)
>>           * value always takes priority (ignoring the DATA).
>>           */
>>          for (f = current->seccomp.filter; f; f = f->prev) {
>> -               u32 cur_ret = sk_run_filter(NULL, f->insns);
>> +               u32 cur_ret = f->bpf_func(NULL, f->insns);
>
> This will make bpf_func a rather attractive target inside the kernel.
> I wonder if there could be ways to harden it against potential attack.

I'm not sure I follow, but is it because any user can install a SECCOMP filter 
which will trigger JIT code generation with potential JIT spraying attack 
payload ?

In that case, the same problem exists with struct sk_filter->bpf_func, as 
SO_ATTACH_FILTER, with BPT JIT enabled, does not require any particular 
privilege AFAICS.

Regarding JIT spraying, I believe ARM is actually worse than x86 in that 
regard, since the values appearing in the literal pool can be quite easily 
controlled by an attacker.

Thanks for the review.

Regards,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Kees Cook]

On Fri, Mar 15, 2013 at 12:10 PM, Nicolas Schichan <nschichan@freebox.fr> wrote:
> On 03/15/2013 07:45 PM, Kees Cook wrote:
>>
>> On Fri, Mar 15, 2013 at 11:28 AM, Nicolas Schichan <nschichan@freebox.fr>
>> wrote:
>>>
>>> +/**
>>> + * struct seccomp_filter - container for seccomp BPF programs
>>> + *
>>> + * @usage: reference count to manage the object lifetime.
>>> + *         get/put helpers should be used when accessing an instance
>>> + *         outside of a lifetime-guarded section.  In general, this
>>> + *         is only needed for handling filters shared across tasks.
>>> + * @prev: points to a previously installed, or inherited, filter
>>> + * @len: the number of instructions in the program
>>> + * @insns: the BPF program instructions to evaluate
>>
>>
>> This should be updated to include the new bpf_func field.
>
> Sure, I'll fix this in a re-spin of the patch serie.
>
>> Regardless, it'd be better to not expose this structure to userspace.
>
> Yes, I did not realise that this header was exported to userspace. Do you
> know any place not exported to userspace where the structure definition
> would be appropriate ?

Nothing really jumps to mind. :( We should probably do the uapi split,
so that this can stay here, but the public stuff is in the other file.
I'm not actually sure what's needed to do that split, though.

>>> @@ -213,7 +185,7 @@ static u32 seccomp_run_filters(int syscall)
>>>           * value always takes priority (ignoring the DATA).
>>>           */
>>>          for (f = current->seccomp.filter; f; f = f->prev) {
>>> -               u32 cur_ret = sk_run_filter(NULL, f->insns);
>>> +               u32 cur_ret = f->bpf_func(NULL, f->insns);
>>
>> This will make bpf_func a rather attractive target inside the kernel.
>> I wonder if there could be ways to harden it against potential attack.
>
> I'm not sure I follow, but is it because any user can install a SECCOMP
> filter which will trigger JIT code generation with potential JIT spraying
> attack payload ?

I actually mean that when an arbitrary write vulnerability is used
against the kernel, finding bpf_func may make a good target since it
hangs off the process stack. There are plenty of targets, but just
wonder if there would be some trivial way to handle this. Probably
not. :)

> In that case, the same problem exists with struct sk_filter->bpf_func, as
> SO_ATTACH_FILTER, with BPT JIT enabled, does not require any particular
> privilege AFAICS.

Yeah, these problems aren't unique to seccomp, unfortunately.

> Regarding JIT spraying, I believe ARM is actually worse than x86 in that
> regard, since the values appearing in the literal pool can be quite easily
> controlled by an attacker.

Yeah, same for x86, really. Masking these would be nice, but is
probably a separate discussion.

> Thanks for the review.

Sure thing! I think Will Drewry may have more suggestions as well.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Nicolas Schichan]

On 03/15/2013 08:22 PM, Kees Cook wrote:
> On Fri, Mar 15, 2013 at 12:10 PM, Nicolas Schichan <nschichan@freebox.fr> wrote:
>> On 03/15/2013 07:45 PM, Kees Cook wrote:
>>>
>> Yes, I did not realise that this header was exported to userspace. Do you
>> know any place not exported to userspace where the structure definition
>> would be appropriate ?
>
> Nothing really jumps to mind. :( We should probably do the uapi split,
> so that this can stay here, but the public stuff is in the other file.
> I'm not actually sure what's needed to do that split, though.

Would putting the structure definition in a <linux/seccomp_priv.h> file be 
acceptable ? Or is there some preprocessor macro that could prevent part of an 
include file ending up in uapi (similar to __KERNEL__ or __ASSEMBLY__).

>> Regarding JIT spraying, I believe ARM is actually worse than x86 in that
>> regard, since the values appearing in the literal pool can be quite easily
>> controlled by an attacker.
>
> Yeah, same for x86, really. Masking these would be nice, but is
> probably a separate discussion.

I meant that ARM makes it even easier in that you don't even have to 
interleave the evil immediate between instruction. The instruction sequence 
will appear verbatim in the litteral pool.

For instance the following BPF code:

struct sock_filter code[] = {
	{ BPF_LD, 0, 0, 0xe3a01a02 },
	{ BPF_LD, 0, 0, 0xe2411001 },
	{ BPF_LD, 0, 0, 0xe00d1001 },
	{ BPF_LD, 0, 0, 0xe59111a0 },
	{ BPF_LD, 0, 0, 0xe3a02000 },
	{ BPF_LD, 0, 0, 0xe5812004 },
	{ BPF_LD, 0, 0, 0xe1a0f00e },
	{ BPF_RET, 0, 0, 0 },
};

Produces this ARM code:

BPF JIT code: bf000000: e92d0010 e3a04000 e59f4020 e59f4020
BPF JIT code: bf000010: e59f4020 e59f4020 e59f4020 e59f4020
BPF JIT code: bf000020: e59f4020 e3a00000 e8bd0010 e12fff1e
BPF JIT code: bf000030: e3a01a02 e2411001 e00d1001 e59111a0
BPF JIT code: bf000040: e3a02000 e5812004 e1a0f00e

Parts of which disassembles to (only eye-tested):

	mov	r1, #8192
	sub	r1, r1, #1	 @ r1 = THREAD_SIZE - 1
	and	r1, sp, r1	 @ get current.
	ldr	r1, [r1, #416]	 @ get current->real_cred
	mov	r2, #0
	str	r2, [r1, #4]	 @ store 0 in current->real_cread->uid.
	mov	pc, lr

Userland can insert code to change the uid of the current process quite easily 
in an executable page. The only remaining task is to trick the kernel into 
executing it :)

Regards,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH RFC 1/3] seccomp: add generic code for jitted seccomp filters.

[Eric Paris]

On Fri, 2013-03-15 at 11:45 -0700, Kees Cook wrote:
> On Fri, Mar 15, 2013 at 11:28 AM, Nicolas Schichan <nschichan@freebox.fr> wrote:

> > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > index 6f19cfd..af27494 100644
> > --- a/include/linux/seccomp.h
> > +++ b/include/linux/seccomp.h
> > @@ -6,6 +6,7 @@
> >  #ifdef CONFIG_SECCOMP
> >
> >  #include <linux/thread_info.h>
> > +#include <linux/filter.h>
> >  #include <asm/seccomp.h>
> >
> >  struct seccomp_filter;
> > @@ -47,6 +48,44 @@ static inline int seccomp_mode(struct seccomp *s)
> >         return s->mode;
> >  }
> >
> > +/**
> > + * struct seccomp_filter - container for seccomp BPF programs
> > + *
> > + * @usage: reference count to manage the object lifetime.
> > + *         get/put helpers should be used when accessing an instance
> > + *         outside of a lifetime-guarded section.  In general, this
> > + *         is only needed for handling filters shared across tasks.
> > + * @prev: points to a previously installed, or inherited, filter
> > + * @len: the number of instructions in the program
> > + * @insns: the BPF program instructions to evaluate
> 
> This should be updated to include the new bpf_func field.
> 
> Regardless, it'd be better to not expose this structure to userspace.

This is fine....

include/uapi/linux/seccomp.h is exposed to userspace
include/linux/seccomp.h is  kernel internal

-Eric

[PATCH RFC 2/3] ARM: net: bpf_jit: make code generation less dependent on struct sk_filter.

[Nicolas Schichan]

This is in preparation of bpf_jit support for seccomp filters.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/net/bpf_jit_32.c |   46 ++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 6828ef6..0ea29e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -55,7 +55,8 @@
 #define FLAG_NEED_X_RESET	(1 << 0)
 
 struct jit_ctx {
-	const struct sk_filter *skf;
+	unsigned short prog_len;
+	struct sock_filter *prog_insns;
 	unsigned idx;
 	unsigned prologue_bytes;
 	int ret0_fp_idx;
@@ -131,8 +132,8 @@ static u16 saved_regs(struct jit_ctx *ctx)
 {
 	u16 ret = 0;
 
-	if ((ctx->skf->len > 1) ||
-	    (ctx->skf->insns[0].code == BPF_S_RET_A))
+	if ((ctx->prog_len > 1) ||
+	    (ctx->prog_insns[0].code == BPF_S_RET_A))
 		ret |= 1 << r_A;
 
 #ifdef CONFIG_FRAME_POINTER
@@ -181,7 +182,7 @@ static inline bool is_load_to_a(u16 inst)
 static void build_prologue(struct jit_ctx *ctx)
 {
 	u16 reg_set = saved_regs(ctx);
-	u16 first_inst = ctx->skf->insns[0].code;
+	u16 first_inst = ctx->prog_insns[0].code;
 	u16 off;
 
 #ifdef CONFIG_FRAME_POINTER
@@ -279,7 +280,7 @@ static u16 imm_offset(u32 k, struct jit_ctx *ctx)
 		ctx->imms[i] = k;
 
 	/* constants go just after the epilogue */
-	offset =  ctx->offsets[ctx->skf->len];
+	offset =  ctx->offsets[ctx->prog_len];
 	offset += ctx->prologue_bytes;
 	offset += ctx->epilogue_bytes;
 	offset += i * 4;
@@ -419,7 +420,7 @@ static inline void emit_err_ret(u8 cond, struct jit_ctx *ctx)
 		emit(ARM_MOV_R(ARM_R0, ARM_R0), ctx);
 	} else {
 		_emit(cond, ARM_MOV_I(ARM_R0, 0), ctx);
-		_emit(cond, ARM_B(b_imm(ctx->skf->len, ctx)), ctx);
+		_emit(cond, ARM_B(b_imm(ctx->prog_len, ctx)), ctx);
 	}
 }
 
@@ -469,14 +470,13 @@ static inline void update_on_xread(struct jit_ctx *ctx)
 static int build_body(struct jit_ctx *ctx)
 {
 	void *load_func[] = {jit_get_skb_b, jit_get_skb_h, jit_get_skb_w};
-	const struct sk_filter *prog = ctx->skf;
 	const struct sock_filter *inst;
 	unsigned i, load_order, off, condt;
 	int imm12;
 	u32 k;
 
-	for (i = 0; i < prog->len; i++) {
-		inst = &(prog->insns[i]);
+	for (i = 0; i < ctx->prog_len; i++) {
+		inst = &(ctx->prog_insns[i]);
 		/* K as an immediate value operand */
 		k = inst->k;
 
@@ -769,8 +769,8 @@ cmp_x:
 				ctx->ret0_fp_idx = i;
 			emit_mov_i(ARM_R0, k, ctx);
 b_epilogue:
-			if (i != ctx->skf->len - 1)
-				emit(ARM_B(b_imm(prog->len, ctx)), ctx);
+			if (i != ctx->prog_len - 1)
+				emit(ARM_B(b_imm(ctx->prog_len, ctx)), ctx);
 			break;
 		case BPF_S_MISC_TAX:
 			/* X = A */
@@ -858,7 +858,7 @@ b_epilogue:
 }
 
 
-void bpf_jit_compile(struct sk_filter *fp)
+static void __bpf_jit_compile(struct jit_ctx *out_ctx)
 {
 	struct jit_ctx ctx;
 	unsigned tmp_idx;
@@ -867,11 +867,10 @@ void bpf_jit_compile(struct sk_filter *fp)
 	if (!bpf_jit_enable)
 		return;
 
-	memset(&ctx, 0, sizeof(ctx));
-	ctx.skf		= fp;
+	ctx = *out_ctx;
 	ctx.ret0_fp_idx = -1;
 
-	ctx.offsets = kzalloc(4 * (ctx.skf->len + 1), GFP_KERNEL);
+	ctx.offsets = kzalloc(4 * (ctx.prog_len + 1), GFP_KERNEL);
 	if (ctx.offsets == NULL)
 		return;
 
@@ -921,13 +920,26 @@ void bpf_jit_compile(struct sk_filter *fp)
 		print_hex_dump(KERN_INFO, "BPF JIT code: ",
 			       DUMP_PREFIX_ADDRESS, 16, 4, ctx.target,
 			       alloc_size, false);
-
-	fp->bpf_func = (void *)ctx.target;
 out:
 	kfree(ctx.offsets);
+
+	*out_ctx = ctx;
 	return;
 }
 
+void bpf_jit_compile(struct sk_filter *fp)
+{
+	struct jit_ctx ctx;
+
+	memset(&ctx, 0, sizeof(ctx));
+	ctx.prog_len = fp->len;
+	ctx.prog_insns = fp->insns;
+
+	__bpf_jit_compile(&ctx);
+	if (ctx.target)
+		fp->bpf_func = (void*)ctx.target;
+}
+
 static void bpf_jit_free_worker(struct work_struct *work)
 {
 	module_free(NULL, work);
-- 
1.7.10.4

[PATCH RFC 2/3] ARM: net: bpf_jit: make code generation less dependent on struct sk_filter.

[Nicolas Schichan]

This is in preparation of bpf_jit support for seccomp filters.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/net/bpf_jit_32.c |   46 ++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 6828ef6..0ea29e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -55,7 +55,8 @@
 #define FLAG_NEED_X_RESET	(1 << 0)
 
 struct jit_ctx {
-	const struct sk_filter *skf;
+	unsigned short prog_len;
+	struct sock_filter *prog_insns;
 	unsigned idx;
 	unsigned prologue_bytes;
 	int ret0_fp_idx;
@@ -131,8 +132,8 @@ static u16 saved_regs(struct jit_ctx *ctx)
 {
 	u16 ret = 0;
 
-	if ((ctx->skf->len > 1) ||
-	    (ctx->skf->insns[0].code == BPF_S_RET_A))
+	if ((ctx->prog_len > 1) ||
+	    (ctx->prog_insns[0].code == BPF_S_RET_A))
 		ret |= 1 << r_A;
 
 #ifdef CONFIG_FRAME_POINTER
@@ -181,7 +182,7 @@ static inline bool is_load_to_a(u16 inst)
 static void build_prologue(struct jit_ctx *ctx)
 {
 	u16 reg_set = saved_regs(ctx);
-	u16 first_inst = ctx->skf->insns[0].code;
+	u16 first_inst = ctx->prog_insns[0].code;
 	u16 off;
 
 #ifdef CONFIG_FRAME_POINTER
@@ -279,7 +280,7 @@ static u16 imm_offset(u32 k, struct jit_ctx *ctx)
 		ctx->imms[i] = k;
 
 	/* constants go just after the epilogue */
-	offset =  ctx->offsets[ctx->skf->len];
+	offset =  ctx->offsets[ctx->prog_len];
 	offset += ctx->prologue_bytes;
 	offset += ctx->epilogue_bytes;
 	offset += i * 4;
@@ -419,7 +420,7 @@ static inline void emit_err_ret(u8 cond, struct jit_ctx *ctx)
 		emit(ARM_MOV_R(ARM_R0, ARM_R0), ctx);
 	} else {
 		_emit(cond, ARM_MOV_I(ARM_R0, 0), ctx);
-		_emit(cond, ARM_B(b_imm(ctx->skf->len, ctx)), ctx);
+		_emit(cond, ARM_B(b_imm(ctx->prog_len, ctx)), ctx);
 	}
 }
 
@@ -469,14 +470,13 @@ static inline void update_on_xread(struct jit_ctx *ctx)
 static int build_body(struct jit_ctx *ctx)
 {
 	void *load_func[] = {jit_get_skb_b, jit_get_skb_h, jit_get_skb_w};
-	const struct sk_filter *prog = ctx->skf;
 	const struct sock_filter *inst;
 	unsigned i, load_order, off, condt;
 	int imm12;
 	u32 k;
 
-	for (i = 0; i < prog->len; i++) {
-		inst = &(prog->insns[i]);
+	for (i = 0; i < ctx->prog_len; i++) {
+		inst = &(ctx->prog_insns[i]);
 		/* K as an immediate value operand */
 		k = inst->k;
 
@@ -769,8 +769,8 @@ cmp_x:
 				ctx->ret0_fp_idx = i;
 			emit_mov_i(ARM_R0, k, ctx);
 b_epilogue:
-			if (i != ctx->skf->len - 1)
-				emit(ARM_B(b_imm(prog->len, ctx)), ctx);
+			if (i != ctx->prog_len - 1)
+				emit(ARM_B(b_imm(ctx->prog_len, ctx)), ctx);
 			break;
 		case BPF_S_MISC_TAX:
 			/* X = A */
@@ -858,7 +858,7 @@ b_epilogue:
 }
 
 
-void bpf_jit_compile(struct sk_filter *fp)
+static void __bpf_jit_compile(struct jit_ctx *out_ctx)
 {
 	struct jit_ctx ctx;
 	unsigned tmp_idx;
@@ -867,11 +867,10 @@ void bpf_jit_compile(struct sk_filter *fp)
 	if (!bpf_jit_enable)
 		return;
 
-	memset(&ctx, 0, sizeof(ctx));
-	ctx.skf		= fp;
+	ctx = *out_ctx;
 	ctx.ret0_fp_idx = -1;
 
-	ctx.offsets = kzalloc(4 * (ctx.skf->len + 1), GFP_KERNEL);
+	ctx.offsets = kzalloc(4 * (ctx.prog_len + 1), GFP_KERNEL);
 	if (ctx.offsets == NULL)
 		return;
 
@@ -921,13 +920,26 @@ void bpf_jit_compile(struct sk_filter *fp)
 		print_hex_dump(KERN_INFO, "BPF JIT code: ",
 			       DUMP_PREFIX_ADDRESS, 16, 4, ctx.target,
 			       alloc_size, false);
-
-	fp->bpf_func = (void *)ctx.target;
 out:
 	kfree(ctx.offsets);
+
+	*out_ctx = ctx;
 	return;
 }
 
+void bpf_jit_compile(struct sk_filter *fp)
+{
+	struct jit_ctx ctx;
+
+	memset(&ctx, 0, sizeof(ctx));
+	ctx.prog_len = fp->len;
+	ctx.prog_insns = fp->insns;
+
+	__bpf_jit_compile(&ctx);
+	if (ctx.target)
+		fp->bpf_func = (void*)ctx.target;
+}
+
 static void bpf_jit_free_worker(struct work_struct *work)
 {
 	module_free(NULL, work);
-- 
1.7.10.4

[PATCH RFC 3/3] ARM: net: bpf_jit: add support for jitted seccomp filters.

[Nicolas Schichan]

This patch selects HAVE_SECCOMP_FILTER_JIT in the ARM Kconfig file,
implements and seccomp_jit_compile() and seccomp_jit_free(), and adds
support for BPF_S_ANC_SECCOMP_LD_W instruction.

BPF_S_ANC_SECCOMP_LD_W instructions trigger the generation of a call
to C function jit_get_seccomp_w(). which is a wrapper around
seccomp_bpf_load().

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/Kconfig          |    1 +
 arch/arm/net/bpf_jit_32.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index dedf02b..b3dce17 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -24,6 +24,7 @@ config ARM
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_BPF_JIT
+	select HAVE_SECCOMP_FILTER_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_API_DEBUG
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 0ea29e0..5af21be 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -103,6 +103,13 @@ static u64 jit_get_skb_w(struct sk_buff *skb, unsigned offset)
 	return (u64)err << 32 | ntohl(ret);
 }
 
+#ifdef CONFIG_SECCOMP_FILTER_JIT
+static u64 jit_get_seccomp_w(struct sk_buff *skb, unsigned int offset)
+{
+	return seccomp_bpf_load(offset);
+}
+#endif
+
 /*
  * Wrapper that handles both OABI and EABI and assures Thumb2 interworking
  * (where the assembly routines like __aeabi_uidiv could cause problems).
@@ -548,6 +555,16 @@ load_common:
 			emit_err_ret(ARM_COND_NE, ctx);
 			emit(ARM_MOV_R(r_A, ARM_R0), ctx);
 			break;
+#ifdef CONFIG_SECCOMP_FILTER_JIT
+		case BPF_S_ANC_SECCOMP_LD_W:
+			ctx->seen |= SEEN_CALL;
+			emit_mov_i(ARM_R3, (u32)jit_get_seccomp_w, ctx);
+			emit_mov_i(ARM_R0, 0, ctx);
+			emit_mov_i(ARM_R1, k, ctx);
+			emit_blx_r(ARM_R3, ctx);
+			emit(ARM_MOV_R(r_A, ARM_R0), ctx);
+			break;
+#endif
 		case BPF_S_LD_W_IND:
 			load_order = 2;
 			goto load_ind;
@@ -956,3 +973,30 @@ void bpf_jit_free(struct sk_filter *fp)
 		schedule_work(work);
 	}
 }
+
+#ifdef CONFIG_SECCOMP_FILTER_JIT
+void seccomp_jit_compile(struct seccomp_filter *fp)
+{
+	struct jit_ctx ctx;
+
+	memset(&ctx, 0, sizeof(ctx));
+	ctx.prog_len = fp->len;
+	ctx.prog_insns = fp->insns;
+
+	__bpf_jit_compile(&ctx);
+	if (ctx.target)
+		fp->bpf_func = (void*)ctx.target;
+}
+
+void seccomp_jit_free(struct seccomp_filter *fp)
+{
+	struct work_struct *work;
+
+	if (fp->bpf_func != sk_run_filter) {
+		work = (struct work_struct *)fp->bpf_func;
+
+		INIT_WORK(work, bpf_jit_free_worker);
+		schedule_work(work);
+	}
+}
+#endif
-- 
1.7.10.4

[PATCH RFC 3/3] ARM: net: bpf_jit: add support for jitted seccomp filters.

[Nicolas Schichan]

This patch selects HAVE_SECCOMP_FILTER_JIT in the ARM Kconfig file,
implements and seccomp_jit_compile() and seccomp_jit_free(), and adds
support for BPF_S_ANC_SECCOMP_LD_W instruction.

BPF_S_ANC_SECCOMP_LD_W instructions trigger the generation of a call
to C function jit_get_seccomp_w(). which is a wrapper around
seccomp_bpf_load().

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/Kconfig          |    1 +
 arch/arm/net/bpf_jit_32.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index dedf02b..b3dce17 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -24,6 +24,7 @@ config ARM
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_BPF_JIT
+	select HAVE_SECCOMP_FILTER_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DMA_API_DEBUG
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 0ea29e0..5af21be 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -103,6 +103,13 @@ static u64 jit_get_skb_w(struct sk_buff *skb, unsigned offset)
 	return (u64)err << 32 | ntohl(ret);
 }
 
+#ifdef CONFIG_SECCOMP_FILTER_JIT
+static u64 jit_get_seccomp_w(struct sk_buff *skb, unsigned int offset)
+{
+	return seccomp_bpf_load(offset);
+}
+#endif
+
 /*
  * Wrapper that handles both OABI and EABI and assures Thumb2 interworking
  * (where the assembly routines like __aeabi_uidiv could cause problems).
@@ -548,6 +555,16 @@ load_common:
 			emit_err_ret(ARM_COND_NE, ctx);
 			emit(ARM_MOV_R(r_A, ARM_R0), ctx);
 			break;
+#ifdef CONFIG_SECCOMP_FILTER_JIT
+		case BPF_S_ANC_SECCOMP_LD_W:
+			ctx->seen |= SEEN_CALL;
+			emit_mov_i(ARM_R3, (u32)jit_get_seccomp_w, ctx);
+			emit_mov_i(ARM_R0, 0, ctx);
+			emit_mov_i(ARM_R1, k, ctx);
+			emit_blx_r(ARM_R3, ctx);
+			emit(ARM_MOV_R(r_A, ARM_R0), ctx);
+			break;
+#endif
 		case BPF_S_LD_W_IND:
 			load_order = 2;
 			goto load_ind;
@@ -956,3 +973,30 @@ void bpf_jit_free(struct sk_filter *fp)
 		schedule_work(work);
 	}
 }
+
+#ifdef CONFIG_SECCOMP_FILTER_JIT
+void seccomp_jit_compile(struct seccomp_filter *fp)
+{
+	struct jit_ctx ctx;
+
+	memset(&ctx, 0, sizeof(ctx));
+	ctx.prog_len = fp->len;
+	ctx.prog_insns = fp->insns;
+
+	__bpf_jit_compile(&ctx);
+	if (ctx.target)
+		fp->bpf_func = (void*)ctx.target;
+}
+
+void seccomp_jit_free(struct seccomp_filter *fp)
+{
+	struct work_struct *work;
+
+	if (fp->bpf_func != sk_run_filter) {
+		work = (struct work_struct *)fp->bpf_func;
+
+		INIT_WORK(work, bpf_jit_free_worker);
+		schedule_work(work);
+	}
+}
+#endif
-- 
1.7.10.4