Re: [Xen-devel] [PATCH v3 2/3] x86/ldt: Make modify_ldt optional

[Willy Tarreau <w@xxxxxx>]

Hi Andy,

On Wed, Jul 22, 2015 at 12:23:47PM -0700, Andy Lutomirski wrote:
> The modify_ldt syscall exposes a large attack surface and is
> unnecessary for modern userspace.  Make it optional.

Wouldn't you prefer something like this which makes it possible to re-enable
it at runtime so that we can hope distros ship with it disabled by default ?

It's pretty efficient on your ldtgdt testcase :

# echo 1 > /proc/sys/kernel/modify_ldt
# ./a.out
[OK]    LDT entry 0 has AR 0x0040FA00 and limit 0x0000000A
[OK]    LDT entry 0 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 1 is invalid
[OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 1 is invalid
[OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D0FA00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07A00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00907A00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07200 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07000 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07400 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00507600 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507E00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507C00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507A00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[RUN]   Test fork
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[OK]    LDT entry 1 is invalid
[OK]    LDT entry 0 has AR 0x0040FA00 and limit 0x0000000A
[OK]    LDT entry 0 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 1 is invalid
[OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 1 is invalid
[OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D0FA00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07A00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00907A00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07200 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07000 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07400 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00507600 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507E00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507C00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507A00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[RUN]   Test fork
[OK]    Child succeeded
[OK]    modify_ldt failure 22
[OK]    LDT entry 0 has AR 0x0000F200 and limit 0x00000000
[OK]    LDT entry 0 has AR 0x00007200 and limit 0x00000000
[OK]    LDT entry 0 has AR 0x0000F000 and limit 0x00000000
[OK]    LDT entry 0 has AR 0x00007200 and limit 0x00000000
[OK]    LDT entry 0 has AR 0x00007000 and limit 0x00000001
[OK]    LDT entry 0 has AR 0x00007000 and limit 0x00000000
[OK]    LDT entry 0 is invalid
[OK]    LDT entry 0 has AR 0x0040F200 and limit 0x00000000
[OK]    LDT entry 0 is invalid
[SKIP]  Cannot set affinity to CPU 1


# echo 0 > /proc/sys/kernel/modify_ldt
# ./a.out
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    LDT entry 1 is invalid
[OK]    modify_ldt is returned -ENOSYS
[OK]    LDT entry 1 is invalid
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[SKIP]  Skipping fork test because have no LDT
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[OK]    modify_ldt is returned -ENOSYS
[SKIP]  Cannot set affinity to CPU 1

The patch is quite small (I stole your comment for the config option).

Willy


diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 226d569..b926f65 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1012,6 +1012,23 @@ config X86_16BIT
          this option saves about 300 bytes on i386, or around 6K text
          plus 16K runtime memory on x86-64,
 
+config DEFAULT_MODIFY_LDT_SYSCALL
+       bool "Allow userspace to modify the LDT (local descriptor table)"
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+         Local Descriptor Table (LDT) using the modify_ldt(2) system
+         call.  This is required to run 16-bit or segmented code such as
+         DOSEMU or some Wine programs.  It is also used by some very old
+         threading libraries.
+
+         Enabling this feature increases the low-level kernel attack
+         surface. Disabling it disables the modify_ldt(2) system call by
+         default. Note that even when disabled it remains possible to
+         enable it at runtime by setting the sys.kernel.modify_ldt sysctl.
+
+         Say 'N' here if you don't expect to use DOSEMU or Wine often.
+
 config X86_ESPFIX32
        def_bool y
        depends on X86_16BIT && X86_32
diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
index c37886d..2f10b6c 100644
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -20,6 +20,12 @@
 #include <asm/mmu_context.h>
 #include <asm/syscalls.h>
 
+#ifdef CONFIG_DEFAULT_MODIFY_LDT_SYSCALL
+int sysctl_modify_ldt __read_mostly = 1;
+#else
+int sysctl_modify_ldt __read_mostly = 0;
+#endif
+
 #ifdef CONFIG_SMP
 static void flush_ldt(void *current_mm)
 {
@@ -254,6 +260,9 @@ asmlinkage int sys_modify_ldt(int func, void __user *ptr,
 {
        int ret = -ENOSYS;
 
+       if (!sysctl_modify_ldt)
+               return ret;
+
        switch (func) {
        case 0:
                ret = read_ldt(ptr, bytecount);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 2082b1a..60270c6 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_X86
+extern int sysctl_modify_ldt;
+#endif
 
 /* Constants used for minimum and  maximum */
 #ifdef CONFIG_LOCKUP_DETECTOR
@@ -962,6 +965,13 @@ static struct ctl_table kern_table[] = {
                .mode           = 0644,
                .proc_handler   = proc_dointvec,
        },
+       {
+               .procname       = "modify_ldt",
+               .data           = &sysctl_modify_ldt,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = proc_dointvec,
+       },
 #endif
 #if defined(CONFIG_MMU)
        {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel