Message-ID: <20150805090311.GB24964@1wt.eu>
Date:	Wed, 5 Aug 2015 11:03:11 +0200
From:	Willy Tarreau <w@....eu>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Andy Lutomirski <luto@...nel.org>,
	Kees Cook <keescook@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	"security@...nel.org" <security@...nel.org>,
	X86 ML <x86@...nel.org>, Borislav Petkov <bp@...en8.de>,
	Sasha Levin <sasha.levin@...cle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	Andrew Cooper <andrew.cooper3@...rix.com>,
	Jan Beulich <jbeulich@...e.com>,
	xen-devel <xen-devel@...ts.xen.org>
Subject: Re: [PATCH v3 1/1] x86: allow to enable/disable modify_ldt at run time

On Wed, Aug 05, 2015 at 10:26:16AM +0200, Ingo Molnar wrote:
> > +modify_ldt: (X86 only)
> > +
> > +Enables (1) or disables (0) the modify_ldt syscall. Modifying the LDT
> > +(Local Descriptor Table) may be needed to run a 16-bit or segmented code
> 
> s/run a/run

good catch, thanks.

> > +such as Dosemu or Wine. This is done via a system call which is not needed
> 
> s/Dosemu/DOSEMU

I didn't know. Fixed.

> > +to run portable applications, and which can sometimes be abused to exploit
> > +some weaknesses of the architecture, opening new vulnerabilities.
> 
> So that's pretty vague IMHO, and a bit FUD-ish in character. How about:
> 
>   ... , and which system call exposes complex, rarely used legacy hardware 
>   features and semantics that had suffered vulnerabilities in the past.

OK fine for me, fixed.

> > +	if (!sysctl_modify_ldt) {
> > +		printk_ratelimited(KERN_INFO
> > +			"Denied a call to modify_ldt() from %s[%d] (uid: %d)."
> > +			" Adjust the modify_ldt sysctl if this was not an"
> 
> Would it really be so difficult to write this as:
> 
>   Set "sys.kernel.modify_ldt = 1" in /etc/sysctl.conf if this was not an exploit attempt.

It's just a matter of taste. Normally I consider the kernel distro-agnostic
so I don't like to suggest one way to adjust sysctls nor to reference config
files. Here we're in a case where only standard distro users may hit the
issue, and users of embedded distros will not face this message or will
easily translate it into their respective configuration scheme. So OK for
this one.

> 99% of the users seeing this message will see it right after an app of theirs 
> ended up not working. Let's not add to the annoyance factor!

That's exactly why I wrote the message in the first place!

> > +			" exploit attempt.\n",
> > +			current->comm, task_pid_nr(current),
> > +			from_kuid_munged(current_user_ns(), current_uid()));
> 
> Also generally please don't break message lines in the source code while they are 
> a single line in the syslog, to make it easier to grep for and to expose kernel 
> hackers to the form of message they are emitting. Ignore checkpatch.

I'm fine with this rule as well, it's only in the kenrel that I tend to
care about the 80-col limit, in my own code I easily ignore it for the
same reason.

> > @@ -960,6 +963,17 @@ static struct ctl_table kern_table[] = {
> >  		.mode		= 0644,
> >  		.proc_handler	= proc_dointvec,
> >  	},
> > +#ifdef CONFIG_MODIFY_LDT_SYSCALL
> > +	{
> > +		.procname	= "modify_ldt",
> > +		.data		= &sysctl_modify_ldt,
> > +		.maxlen		= sizeof(int),
> > +		.mode		= 0644,
> > +		.proc_handler   = proc_dointvec_minmax,
> > +		.extra1         = &zero,
> > +		.extra2         = &one,
> > +	},
> > +#endif
> 
> So I'd actually make the permissions 0600: to make it a tiny bit harder for 
> exploits to silently query the current value to figure out whether they can safely 
> attempt the syscall or not ...

That's a good point. If we later make other syscalls configurable, it might
be different because some users might want to contact their admin to ask for
a specific one. But here, there's usually no admin so I'm fine with hardening
it.

> (Sadly /etc/sysctl.conf is world-readable on most distros.)

Yes, just like most executables are readable while not strictly needed,
especially setuid ones which allow the code to be studied in place or
easily duplicated to script some exploits. But we could discuss hours
about basic system hardening!

I've updated the patch with your suggestions.

Thanks,
Willy


>From 8521f170515dfc0f390c396100140504c8dfbcfc Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@....eu>
Date: Sat, 25 Jul 2015 12:18:33 +0200
Subject: [PATCH] x86/ldt: allow to disable modify_ldt at runtime

For distros who prefer not to take the risk of completely disabling the
modify_ldt syscall using CONFIG_MODIFY_LDT_SYSCALL, this patch adds a
sysctl to enable or/disable it at runtime, and proposes to disable it
by default. This can be a safe alternative. A message is logged if an
attempt was stopped so that it's easy to spot if/when it is needed.

Cc: Andy Lutomirski <luto@...nel.org>
Cc: Kees Cook <keescook@...omium.org>
Cc: Ingo Molnar <mingo@...nel.org>
Signed-off-by: Willy Tarreau <w@....eu>
---
 Documentation/sysctl/kernel.txt | 16 ++++++++++++++++
 arch/x86/Kconfig                | 17 +++++++++++++++++
 arch/x86/kernel/ldt.c           | 15 +++++++++++++++
 kernel/sysctl.c                 | 14 ++++++++++++++
 4 files changed, 62 insertions(+)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 6fccb69..7dcdebd 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -41,6 +41,7 @@ show up in /proc/sys/kernel:
 - kptr_restrict
 - kstack_depth_to_print       [ X86 only ]
 - l2cr                        [ PPC only ]
+- modify_ldt                  [ X86 only ]
 - modprobe                    ==> Documentation/debugging-modules.txt
 - modules_disabled
 - msg_next_id		      [ sysv ipc ]
@@ -391,6 +392,21 @@ This flag controls the L2 cache of G3 processor boards. If
 
 ==============================================================
 
+modify_ldt: (X86 only)
+
+Enables (1) or disables (0) the modify_ldt syscall. Modifying the LDT
+(Local Descriptor Table) may be needed to run 16-bit or segmented code
+such as DOSEMU or Wine. This is done via a system call which is not needed
+to run portable applications, and which exposes complex, rarely used legacy
+hardware features and semantics that had suffered vulnerabilities in the
+past.
+
+This sysctl allows one to increase the system's security by disabling the
+system call, or to restore compatibility with specific applications when it
+was already disabled.
+
+==============================================================
+
 modules_disabled:
 
 A toggle value indicating if modules are allowed to be loaded
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index beabf30..e528fb0 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2069,6 +2069,23 @@ config MODIFY_LDT_SYSCALL
 	  surface.  Disabling it removes the modify_ldt(2) system call.
 
 	  Saying 'N' here may make sense for embedded or server kernels.
+	  If really unsure, say 'Y', you'll be able to disable it at runtime.
+
+config DEFAULT_MODIFY_LDT_SYSCALL
+	bool "Allow userspace to modify the LDT by default"
+	depends on MODIFY_LDT_SYSCALL
+	default y
+	---help---
+	  Modifying the LDT (Local Descriptor Table) may be needed to run
+	  16-bit or segmented code such as DOSEMU or Wine. This is done via
+	  a system call which is not needed to run portable applications,
+	  and which exposes complex, rarely used legacy hardware features
+	  and semantics that had suffered vulnerabilities in the past.
+
+	  For this reason this option allows one to enable or disable the
+	  feature at runtime. It is recommended to say 'N' here to leave
+	  the system protected, and to enable it at runtime only if needed
+	  by setting the sys.kernel.modify_ldt sysctl.
 
 source "kernel/livepatch/Kconfig"
 
diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
index 2bcc052..b8a4f2d 100644
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -11,6 +11,7 @@
 #include <linux/sched.h>
 #include <linux/string.h>
 #include <linux/mm.h>
+#include <linux/ratelimit.h>
 #include <linux/smp.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
@@ -21,6 +22,11 @@
 #include <asm/mmu_context.h>
 #include <asm/syscalls.h>
 
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
+int sysctl_modify_ldt __read_mostly =
+	IS_ENABLED(CONFIG_DEFAULT_MODIFY_LDT_SYSCALL);
+#endif
+
 /* context.lock is held for us, so we don't need any locking. */
 static void flush_ldt(void *current_mm)
 {
@@ -276,6 +282,15 @@ asmlinkage int sys_modify_ldt(int func, void __user *ptr,
 {
 	int ret = -ENOSYS;
 
+	if (!sysctl_modify_ldt) {
+		printk_ratelimited(KERN_INFO
+			"Denied a call to modify_ldt() from %s[%d] (uid: %d)."
+			" Set \"sys.kernel.modify_ldt = 1\" in /etc/sysctl.conf if this was not an exploit attempt.\n",
+			current->comm, task_pid_nr(current),
+			from_kuid_munged(current_user_ns(), current_uid()));
+		return ret;
+	}
+
 	switch (func) {
 	case 0:
 		ret = read_ldt(ptr, bytecount);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 19b62b5..f7eb41f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -111,6 +111,9 @@ extern int sysctl_nr_open_min, sysctl_nr_open_max;
 #ifndef CONFIG_MMU
 extern int sysctl_nr_trim_pages;
 #endif
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
+extern int sysctl_modify_ldt;
+#endif
 
 /* Constants used for minimum and  maximum */
 #ifdef CONFIG_LOCKUP_DETECTOR
@@ -960,6 +963,17 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
+	{
+		.procname	= "modify_ldt",
+		.data		= &sysctl_modify_ldt,
+		.maxlen		= sizeof(int),
+		.mode		= 0600,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1         = &zero,
+		.extra2         = &one,
+	},
+#endif
 #endif
 #if defined(CONFIG_MMU)
 	{
-- 
1.7.12.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists