public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* Re: [patch] let CONFIG_SECCOMP default to n
@ 2006-07-13  5:43 Albert Cahalan
  2006-07-13  7:04 ` utrace vs. ptrace Ingo Molnar
  2006-07-13  7:07 ` [patch] let CONFIG_SECCOMP default to n andrea
  0 siblings, 2 replies; 60+ messages in thread
From: Albert Cahalan @ 2006-07-13  5:43 UTC (permalink / raw)
  To: torvalds, andrea, ak, mingo, alan, arjan, bunk, akpm, rlrevell,
	linux-kernel

Linus Torvalds writes:

> I don't think SECCOMP is wrong per se, but I do believe that
> if other approaches become more popular, and the only user of
> SECCOMP is not GPL'd and uses some patented stuff, then we should
> seriously look at the other interfaces (eg the extended ptrace).
>
> Does anybody actually really _use_ SECCOMP outside of the
> patented stuff?

I write debugger code. I can not possibly express how broken
the ptrace interface is. There are numerous corner conditions
that it gets terribly wrong. If you single step over any
"interesting" instructions, if the target plays funny games
with signals or the trap flag...

The utrace stuff offers some hope for eventually fixing this
mess. Please accept that or something similar.

As for SECCOMP... non-root users need high-performance ways
to sandbox things. I do not believe that one solution fits all.
Perhaps SE Linux could be extended to let users sub-divide
their accounts, and certainly ptrace could be made better.

SECCOMP is a good idea, but currently a tad too limiting.
There are a few dozen system calls that would be safe and useful,
particularly those related to signals, memory, and synchronization.

I see no reason to have a config option outside of
CONFIG_EMBEDDED. Ditch the TSC stuff though.

^ permalink raw reply	[flat|nested] 60+ messages in thread
[parent not found: <6tgj0-8ip-19@gated-at.bofh.it>]
* Re: [patch] let CONFIG_SECCOMP default to n
@ 2006-07-12 21:37 Chuck Ebbert
  2006-07-12 21:55 ` Linus Torvalds
  2006-07-12 21:57 ` Andi Kleen
  0 siblings, 2 replies; 60+ messages in thread
From: Chuck Ebbert @ 2006-07-12 21:37 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Alan Cox, Arjan van de Ven, Ingo Molnar, Adrian Bunk,
	linux-kernel, Linus Torvalds, andrea

In-Reply-To: <p73wtain80h.fsf@verdi.suse.de>

On 12 Jul 2006 17:43:42 +0200, Andi Kleen wrote:

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> > 
> > I really don't care about cpushare and patents for some users of the
> > code in question. On the other hand turning on performance harming code
> > for a tiny number of users is dumb. If it were a loadable module it
> > would be different.
> 
> Actually there are some promising applications of seccomp outside
> cpushare.
> 
> e.g. Andrea at some point proposed to run codecs which often
> have security issues in a simple cpusec jail.  That's ok for 
> them because they normally don't need to do any system calls.
> 
> I liked the idea. While this can be done with LSM (e.g. apparmor) too 
> seccomp is definitely much easier and simpler and more "obviously safe"
> than anything LSM based.
> 
> If the TSC disabling code is taken out the runtime overhead
> of seccomp is also very small because it's only tested in slow
> paths.

We can just fold the TSC disable stuff into the new thread_flags test
at context-switch time:

[compile tested only; requires just-sent fix to i386 system.h]

 arch/i386/kernel/process.c     |   61 ++++++++++++++---------------------------
 include/asm-i386/thread_info.h |    3 +-
 2 files changed, 24 insertions(+), 40 deletions(-)

--- 2.6.18-rc1-nb.orig/arch/i386/kernel/process.c
+++ 2.6.18-rc1-nb/arch/i386/kernel/process.c
@@ -535,12 +535,11 @@ int dump_task_regs(struct task_struct *t
 	return 1;
 }
 
-static noinline void __switch_to_xtra(struct task_struct *next_p,
-				    struct tss_struct *tss)
+static noinline void
+__switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
+		 struct tss_struct *tss)
 {
-	struct thread_struct *next;
-
-	next = &next_p->thread;
+	struct thread_struct *next = &next_p->thread;
 
 	if (test_tsk_thread_flag(next_p, TIF_DEBUG)) {
 		set_debugreg(next->debugreg[0], 0);
@@ -552,6 +551,19 @@ static noinline void __switch_to_xtra(st
 		set_debugreg(next->debugreg[7], 7);
 	}
 
+	/*
+	 * Context switch may need to tweak the TSC disable bit in CR4.
+	 * The optimizer should remove this code when !CONFIG_SECCOMP.
+	 */
+	if (has_secure_computing(task_thread_info(prev_p)) ^
+	    has_secure_computing(task_thread_info(next_p))) {
+		/* prev and next are different */
+		if (has_secure_computing(task_thread_info(next_p)))
+			write_cr4(read_cr4() | X86_CR4_TSD);
+		else
+			write_cr4(read_cr4() & ~X86_CR4_TSD);
+	}
+
 	if (!test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
 		/*
 		 * Disable the bitmap via an invalid offset. We still cache
@@ -561,7 +573,7 @@ static noinline void __switch_to_xtra(st
 		return;
 	}
 
-	if (likely(next == tss->io_bitmap_owner)) {
+	if (likely(tss->io_bitmap_owner == next)) {
 		/*
 		 * Previous owner of the bitmap (hence the bitmap content)
 		 * matches the next task, we dont have to do anything but
@@ -583,33 +595,6 @@ static noinline void __switch_to_xtra(st
 }
 
 /*
- * This function selects if the context switch from prev to next
- * has to tweak the TSC disable bit in the cr4.
- */
-static inline void disable_tsc(struct task_struct *prev_p,
-			       struct task_struct *next_p)
-{
-	struct thread_info *prev, *next;
-
-	/*
-	 * gcc should eliminate the ->thread_info dereference if
-	 * has_secure_computing returns 0 at compile time (SECCOMP=n).
-	 */
-	prev = task_thread_info(prev_p);
-	next = task_thread_info(next_p);
-
-	if (has_secure_computing(prev) || has_secure_computing(next)) {
-		/* slow path here */
-		if (has_secure_computing(prev) &&
-		    !has_secure_computing(next)) {
-			write_cr4(read_cr4() & ~X86_CR4_TSD);
-		} else if (!has_secure_computing(prev) &&
-			   has_secure_computing(next))
-			write_cr4(read_cr4() | X86_CR4_TSD);
-	}
-}
-
-/*
  *	switch_to(x,yn) should switch tasks from x to y.
  *
  * We fsave/fwait so that an exception goes off at the right time
@@ -688,13 +673,11 @@ struct task_struct fastcall * __switch_t
 		set_iopl_mask(next->iopl);
 
 	/*
-	 * Now maybe handle debug registers and/or IO bitmaps
+	 * Now maybe handle debug registers, IO bitmaps, TSC disable
 	 */
-	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
-	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
-		__switch_to_xtra(next_p, tss);
-
-	disable_tsc(prev_p, next_p);
+	if (unlikely(task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV ||
+		     task_thread_info(next_p)->flags & _TIF_WORK_CTXSW_NEXT))
+		__switch_to_xtra(prev_p, next_p, tss);
 
 	return prev_p;
 }
--- 2.6.18-rc1-nb.orig/include/asm-i386/thread_info.h
+++ 2.6.18-rc1-nb/include/asm-i386/thread_info.h
@@ -164,7 +164,8 @@ static inline struct thread_info *curren
 #define _TIF_ALLWORK_MASK	(0x0000FFFF & ~_TIF_SECCOMP)
 
 /* flags to check in __switch_to() */
-#define _TIF_WORK_CTXSW (_TIF_DEBUG|_TIF_IO_BITMAP)
+#define _TIF_WORK_CTXSW_NEXT (_TIF_IO_BITMAP | _TIF_SECCOMP | _TIF_DEBUG)
+#define _TIF_WORK_CTXSW_PREV (_TIF_IO_BITMAP | _TIF_SECCOMP)
 
 /*
  * Thread-synchronous status.
-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

^ permalink raw reply	[flat|nested] 60+ messages in thread
* [2.6 patch] let CONFIG_SECCOMP default to n
@ 2006-06-29 19:21 Adrian Bunk
  2006-06-30  0:44 ` Lee Revell
  0 siblings, 1 reply; 60+ messages in thread
From: Adrian Bunk @ 2006-06-29 19:21 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, Ingo Molnar

From: Ingo Molnar <mingo@elte.hu>

I was profiling the scheduler on x86 and noticed some overhead related 
to SECCOMP, and indeed, SECCOMP runs disable_tsc() at _every_ 
context-switch:

        if (unlikely(prev->io_bitmap_ptr || next->io_bitmap_ptr))
                handle_io_bitmap(next, tss);

        disable_tsc(prev_p, next_p);

        return prev_p;

these are a couple of instructions in the hottest scheduler codepath!

x86_64 already removed disable_tsc() from switch_to(), but i think the 
right solution is to turn SECCOMP off by default.

besides the runtime overhead, there are a couple of other reasons as 
well why this should be done:

 - CONFIG_SECCOMP=y adds 836 bytes of bloat to the kernel:

       text    data     bss     dec     hex filename
    4185360  867112  391012 5443484  530f9c vmlinux-noseccomp
    4185992  867316  391012 5444320  5312e0 vmlinux-seccomp

 - virtually nobody seems to be using it (but cpushare.com, which seems
   pretty inactive)

 - users/distributions can still turn it on if they want it

 - http://www.cpushare.com/legal seems to suggest that it is pursuing a
   software patent to utilize the seccomp concept in a distributed 
   environment, and seems to give a promise that 'end users' will not be
   affected by that patent. How about non-end-users [i.e. server-side]?
   Has the Linux kernel become a vehicle for a propriety server-side
   feature, with every Linux user paying the price of it?

so the patch below just does the minimal common-sense change: turn it 
off by default.

Adrian Bunk:
I've removed the superfluous "default n"'s the original patch introduced.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Adrian Bunk <bunk@stusta.de>

----

This patch was already sent on:
- 26 Jun 2006
- 27 Apr 2006
- 19 Apr 2006
- 11 Apr 2006
- 10 Mar 2006
- 29 Jan 2006
- 21 Jan 2006

This patch was sent by Ingo Molnar on:
- 9 Jan 2006

Index: linux/arch/i386/Kconfig
===================================================================
--- linux.orig/arch/i386/Kconfig
+++ linux/arch/i386/Kconfig
@@ -637,7 +637,6 @@ config REGPARM
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	depends on PROC_FS
-	default y
 	help
 	  This kernel feature is useful for number crunching applications
 	  that may need to compute untrusted bytecode during their
Index: linux/arch/mips/Kconfig
===================================================================
--- linux.orig/arch/mips/Kconfig
+++ linux/arch/mips/Kconfig
@@ -1787,7 +1787,6 @@ config BINFMT_ELF32
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	depends on PROC_FS && BROKEN
-	default y
 	help
 	  This kernel feature is useful for number crunching applications
 	  that may need to compute untrusted bytecode during their
Index: linux/arch/powerpc/Kconfig
===================================================================
--- linux.orig/arch/powerpc/Kconfig
+++ linux/arch/powerpc/Kconfig
@@ -666,7 +666,6 @@ endif
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	depends on PROC_FS
-	default y
 	help
 	  This kernel feature is useful for number crunching applications
 	  that may need to compute untrusted bytecode during their
Index: linux/arch/ppc/Kconfig
===================================================================
--- linux.orig/arch/ppc/Kconfig
+++ linux/arch/ppc/Kconfig
@@ -1127,7 +1127,6 @@ endif
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	depends on PROC_FS
-	default y
 	help
 	  This kernel feature is useful for number crunching applications
 	  that may need to compute untrusted bytecode during their
Index: linux/arch/sparc64/Kconfig
===================================================================
--- linux.orig/arch/sparc64/Kconfig
+++ linux/arch/sparc64/Kconfig
@@ -64,7 +64,6 @@ endchoice
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	depends on PROC_FS
-	default y
 	help
 	  This kernel feature is useful for number crunching applications
 	  that may need to compute untrusted bytecode during their
Index: linux/arch/x86_64/Kconfig
===================================================================
--- linux.orig/arch/x86_64/Kconfig
+++ linux/arch/x86_64/Kconfig
@@ -466,7 +466,6 @@ config PHYSICAL_START
 config SECCOMP
 	bool "Enable seccomp to safely compute untrusted bytecode"
 	depends on PROC_FS
-	default y
 	help
 	  This kernel feature is useful for number crunching applications
 	  that may need to compute untrusted bytecode during their


^ permalink raw reply	[flat|nested] 60+ messages in thread

end of thread, other threads:[~2006-07-26  0:20 UTC | newest]

Thread overview: 60+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-07-13  5:43 [patch] let CONFIG_SECCOMP default to n Albert Cahalan
2006-07-13  7:04 ` utrace vs. ptrace Ingo Molnar
2006-07-13  9:24   ` Ingo Molnar
2006-07-13 12:37     ` Andi Kleen
2006-07-13 12:43       ` Ingo Molnar
2006-07-13 13:21         ` Andi Kleen
2006-07-13 13:28           ` Arjan van de Ven
2006-07-13 13:34             ` Andi Kleen
2006-07-13 13:37               ` Arjan van de Ven
2006-07-13 13:46                 ` Andi Kleen
2006-07-13 19:05           ` Linus Torvalds
2006-07-13 19:47             ` Ingo Molnar
2006-07-14 10:42               ` Paul Jackson
2006-07-25 18:49             ` Alan Cox
2006-07-25 18:27               ` Linus Torvalds
2006-07-25 18:57                 ` Olaf Hering
2006-07-25 19:12                   ` Ingo Molnar
2006-07-26  0:20               ` Martin Bligh
2006-07-13  7:07 ` [patch] let CONFIG_SECCOMP default to n andrea
     [not found] <6tgj0-8ip-19@gated-at.bofh.it>
     [not found] ` <6xP8s-5mc-9@gated-at.bofh.it>
     [not found]   ` <6xUhQ-4Wx-33@gated-at.bofh.it>
     [not found]     ` <6xVdX-6oH-53@gated-at.bofh.it>
     [not found]       ` <6xVnz-6AI-21@gated-at.bofh.it>
     [not found]         ` <6xZUd-4Es-13@gated-at.bofh.it>
     [not found]           ` <6y7yy-7ws-13@gated-at.bofh.it>
     [not found]             ` <6y7RK-7TX-9@gated-at.bofh.it>
2006-07-17 11:37               ` Bodo Eggert
  -- strict thread matches above, loose matches on Subject: below --
2006-07-12 21:37 Chuck Ebbert
2006-07-12 21:55 ` Linus Torvalds
2006-07-12 22:48   ` andrea
2006-07-12 21:57 ` Andi Kleen
2006-06-29 19:21 [2.6 patch] " Adrian Bunk
2006-06-30  0:44 ` Lee Revell
2006-06-30  1:07   ` Andrew Morton
2006-06-30  1:40     ` Adrian Bunk
2006-06-30  4:52       ` Andrea Arcangeli
2006-06-30  9:47         ` Ingo Molnar
2006-06-30 14:58           ` andrea
2006-07-11  7:36             ` [patch] " Ingo Molnar
2006-07-11 14:17               ` andrea
2006-07-11 14:32                 ` Arjan van de Ven
2006-07-11 15:31                   ` andrea
2006-07-11 15:54                     ` Arjan van de Ven
2006-07-11 16:13                       ` andrea
2006-07-11 16:23                         ` Arjan van de Ven
2006-07-11 16:57                         ` Alan Cox
2006-07-11 16:25                       ` Alan Cox
2006-07-11 16:02                     ` Adrian Bunk
2006-07-11 16:16                       ` andrea
2006-07-11 16:24                     ` Alan Cox
2006-07-12 15:43                       ` Andi Kleen
2006-07-12 21:07                         ` Ingo Molnar
2006-07-12 22:06                           ` Andi Kleen
2006-07-12 22:19                             ` Ingo Molnar
2006-07-12 22:33                               ` Andi Kleen
2006-07-12 22:49                                 ` Ingo Molnar
2006-07-13  3:16                               ` Andrea Arcangeli
2006-07-13 11:23                                 ` Jeff Dike
2006-07-13 11:35                                   ` Ingo Molnar
2006-07-13  3:04                             ` Andrea Arcangeli
2006-07-13  3:12                               ` Linus Torvalds
2006-07-13  4:40                                 ` Andrea Arcangeli
2006-07-13  4:51                                   ` andrea
2006-07-13  5:12                                   ` Linus Torvalds
2006-07-13  6:22                                     ` andrea
2006-07-13  1:51                           ` Andrew Morton
2006-07-13  2:00                             ` Linus Torvalds
2006-07-13  7:44                             ` James Bruce
2006-07-13  8:34                               ` andrea
2006-07-13  9:18                                 ` Andrew Morton
2006-07-13 12:13                             ` Andi Kleen
2006-07-12 21:22                         ` Ingo Molnar
2006-07-12 22:11                           ` Andi Kleen
2006-07-11 15:54                 ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox