[RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Dave Hansen]

This is 3.20 material.  I'm hoping to get some comments early
in case folks have some issues with the way it's being done.

--

The MPX hardware structures differ in layout in 32 and 64-bit
mode.  A 32-bit binary running on a 64-bit kernel needs the
32-bit structures, so we need code which switches between
the two modes.

The first patch is just a documentation update.  The next 5
patches are really just prepwork.  The meat is in the last 3.

	x86: make is_64bit_mm() widely available
	x86: make __VIRTUAL_MASK safe to use on 32 bit
	x86, mpx: we do not allocate the bounds directory
	x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK
	x86, mpx: Add temporary variable to reduce masking
	x86, mpx: new directory entry to addr helper
	x86, mpx: do 32-bit-only cmpxchg for 32-bit apps
	x86, mpx: support 32bit binaries on 64bit kernel

 Documentation/x86/intel_mpx.txt    |   12 +++-
 arch/x86/include/asm/mmu_context.h |   13 +++++
 arch/x86/include/asm/mpx.h         |    2 
 arch/x86/include/asm/page_types.h  |    8 +++
 arch/x86/kernel/uprobes.c          |   10 ---
 arch/x86/mm/mpx.c                  |   93 ++++++++++++++++++++++++++++++-------
 6 files changed, 107 insertions(+), 31 deletions(-)

[RFC][PATCH 1/8] x86: make is_64bit_mm() widely available

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The uprobes code has a nice helper, is_64bit_mm(), that consults both
the runtime and compile-time flags for 32-bit support.  Instead of
reinventing the wheel, pull it in to an x86 header so we can use it
for MPX.

I prefer passing the mm around to test_thread_flag(TIF_IA32) beacuse
it makes it explicit where the context is coming from.  It will also
make it easier if we ever need to access the MPX structures from
another process.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---

 b/arch/x86/include/asm/mmu_context.h |   13 +++++++++++++
 b/arch/x86/kernel/uprobes.c          |   10 +---------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff -puN arch/x86/include/asm/mmu_context.h~x86-make-is_64bit_mm-available arch/x86/include/asm/mmu_context.h
--- a/arch/x86/include/asm/mmu_context.h~x86-make-is_64bit_mm-available	2014-12-12 11:11:39.086947548 -0800
+++ b/arch/x86/include/asm/mmu_context.h	2014-12-12 11:11:39.091947773 -0800
@@ -121,6 +121,19 @@ static inline void arch_exit_mmap(struct
 	paravirt_arch_exit_mmap(mm);
 }
 
+#ifdef CONFIG_X86_64
+static inline bool is_64bit_mm(struct mm_struct *mm)
+{
+	return	!config_enabled(CONFIG_IA32_EMULATION) ||
+		!(mm->context.ia32_compat == TIF_IA32);
+}
+#else
+static inline bool is_64bit_mm(struct mm_struct *mm)
+{
+	return false;
+}
+#endif
+
 static inline void arch_bprm_mm_init(struct mm_struct *mm,
 		struct vm_area_struct *vma)
 {
diff -puN arch/x86/kernel/uprobes.c~x86-make-is_64bit_mm-available arch/x86/kernel/uprobes.c
--- a/arch/x86/kernel/uprobes.c~x86-make-is_64bit_mm-available	2014-12-12 11:11:39.087947593 -0800
+++ b/arch/x86/kernel/uprobes.c	2014-12-12 11:11:39.091947773 -0800
@@ -29,6 +29,7 @@
 #include <linux/kdebug.h>
 #include <asm/processor.h>
 #include <asm/insn.h>
+#include <asm/mmu_context.h>
 
 /* Post-execution fixups. */
 
@@ -245,11 +246,6 @@ static int uprobe_init_insn(struct arch_
 }
 
 #ifdef CONFIG_X86_64
-static inline bool is_64bit_mm(struct mm_struct *mm)
-{
-	return	!config_enabled(CONFIG_IA32_EMULATION) ||
-		!(mm->context.ia32_compat == TIF_IA32);
-}
 /*
  * If arch_uprobe->insn doesn't use rip-relative addressing, return
  * immediately.  Otherwise, rewrite the instruction so that it accesses
@@ -430,10 +426,6 @@ static void riprel_post_xol(struct arch_
 	}
 }
 #else /* 32-bit: */
-static inline bool is_64bit_mm(struct mm_struct *mm)
-{
-	return false;
-}
 /*
  * No RIP-relative addressing on 32-bit
  */
_

[RFC][PATCH 2/8] x86: make __VIRTUAL_MASK safe to use on 32 bit

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

We are going to do some calculations in a moment that are based on the
size of the virtual address space.  __VIRTUAL_MASK is currently unsafe
to use on 32-bit since it overflows an unsigned long with its shift.

The current version will emit a warning if used at all on 32-bit
kernels.

Add a version which is safe on 32-bit and consequently does not spit
out a warning.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/page_types.h |    8 ++++++++
 1 file changed, 8 insertions(+)

diff -puN arch/x86/include/asm/page_types.h~x86-make-__VIRTUAL_MASK-safe-to-use-on-32-bit arch/x86/include/asm/page_types.h
--- a/arch/x86/include/asm/page_types.h~x86-make-__VIRTUAL_MASK-safe-to-use-on-32-bit	2014-12-12 11:11:39.484965499 -0800
+++ b/arch/x86/include/asm/page_types.h	2014-12-12 11:11:39.487965634 -0800
@@ -10,7 +10,15 @@
 #define PAGE_MASK	(~(PAGE_SIZE-1))
 
 #define __PHYSICAL_MASK		((phys_addr_t)((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
+#ifdef CONFIG_X86_64
+/*
+ * This version doesn't work on 32-bit because __VIRTUAL_MASK_SHIFT=32
+ * overflows the unsigned long we're trying to shift.
+ */
 #define __VIRTUAL_MASK		((1UL << __VIRTUAL_MASK_SHIFT) - 1)
+#else
+#define __VIRTUAL_MASK		(~0UL)
+#endif
 
 /* Cast PAGE_MASK to a signed type so that it is sign-extended if
    virtual addresses are 32-bits but physical addresses are larger
_

[RFC][PATCH 3/8] x86, mpx: we do not allocate the bounds directory

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The comment and code here are confusing.  We do not currently
allocate the bounds directory in the kernel.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/mm/mpx.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN arch/x86/mm/mpx.c~x86-mpx-we-do-not-allocate-the-bounds-directory arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~x86-mpx-we-do-not-allocate-the-bounds-directory	2014-12-12 11:11:39.858982368 -0800
+++ b/arch/x86/mm/mpx.c	2014-12-12 11:11:39.861982503 -0800
@@ -47,8 +47,8 @@ static unsigned long mpx_mmap(unsigned l
 	vm_flags_t vm_flags;
 	struct vm_area_struct *vma;
 
-	/* Only bounds table and bounds directory can be allocated here */
-	if (len != MPX_BD_SIZE_BYTES && len != MPX_BT_SIZE_BYTES)
+	/* Only bounds table can be allocated here */
+	if (len != MPX_BT_SIZE_BYTES)
 		return -EINVAL;
 
 	down_write(&mm->mmap_sem);
_

[RFC][PATCH 4/8] x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK

[Dave Hansen]

From: Qiaowei Ren <qiaowei.ren@intel.com>

MPX_BNDCFG_ADDR_MASK is defined two times, so this patch removes
redundant one.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/mpx.h |    1 -
 1 file changed, 1 deletion(-)

diff -puN arch/x86/include/asm/mpx.h~0001-x86-mpx-remove-redundant-MPX_BNDCFG_ADDR_MASK arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~0001-x86-mpx-remove-redundant-MPX_BNDCFG_ADDR_MASK	2014-12-12 11:11:40.228999056 -0800
+++ b/arch/x86/include/asm/mpx.h	2014-12-12 11:11:40.231999192 -0800
@@ -45,7 +45,6 @@
 #define MPX_BNDSTA_TAIL		2
 #define MPX_BNDCFG_TAIL		12
 #define MPX_BNDSTA_ADDR_MASK	(~((1UL<<MPX_BNDSTA_TAIL)-1))
-#define MPX_BNDCFG_ADDR_MASK	(~((1UL<<MPX_BNDCFG_TAIL)-1))
 #define MPX_BT_ADDR_MASK	(~((1UL<<MPX_BD_ENTRY_TAIL)-1))
 
 #define MPX_BNDCFG_ADDR_MASK	(~((1UL<<MPX_BNDCFG_TAIL)-1))
_

[RFC][PATCH 5/8] x86, mpx: Add temporary variable to reduce masking

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

When we allocate a bounds table, we call mmap(), then add a
"valid" bit to the value before storing it in to the bounds
directory.

If we fail along the way, we go and mask that valid bit
_back_ out.  That seems a little silly, and this makes it
much more clear when we have a plain address versus an
actual table _entry_.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/mm/mpx.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff -puN arch/x86/mm/mpx.c~mpx-remove-unnecessary-masking arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~mpx-remove-unnecessary-masking	2014-12-12 11:11:40.599015745 -0800
+++ b/arch/x86/mm/mpx.c	2014-12-12 11:11:40.602015880 -0800
@@ -421,6 +421,7 @@ static int allocate_bt(long __user *bd_e
 	unsigned long expected_old_val = 0;
 	unsigned long actual_old_val = 0;
 	unsigned long bt_addr;
+	unsigned long bd_new_entry;
 	int ret = 0;
 
 	/*
@@ -433,7 +434,7 @@ static int allocate_bt(long __user *bd_e
 	/*
 	 * Set the valid flag (kinda like _PAGE_PRESENT in a pte)
 	 */
-	bt_addr = bt_addr | MPX_BD_ENTRY_VALID_FLAG;
+	bd_new_entry = bt_addr | MPX_BD_ENTRY_VALID_FLAG;
 
 	/*
 	 * Go poke the address of the new bounds table in to the
@@ -447,7 +448,7 @@ static int allocate_bt(long __user *bd_e
 	 * of the MPX code that have to pagefault_disable().
 	 */
 	ret = user_atomic_cmpxchg_inatomic(&actual_old_val, bd_entry,
-					   expected_old_val, bt_addr);
+					   expected_old_val, bd_new_entry);
 	if (ret)
 		goto out_unmap;
 
@@ -477,7 +478,7 @@ static int allocate_bt(long __user *bd_e
 	}
 	return 0;
 out_unmap:
-	vm_munmap(bt_addr & MPX_BT_ADDR_MASK, MPX_BT_SIZE_BYTES);
+	vm_munmap(bt_addr, MPX_BT_SIZE_BYTES);
 	return ret;
 }
 
_

[RFC][PATCH 6/8] x86, mpx: new directory entry to addr helper

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Currently, to get from a bounds directory entry to the virtual
address of a bounds table, we simply mask off a few low bits.
However, the set of bits we mask off is different for 32 and
64-bit binaries.

This breaks the operation out in to a helper function and also
adds a temporary variable to store the result until we are
sure we are returning one.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/mpx.h |    1 -
 b/arch/x86/mm/mpx.c          |   41 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff -puN arch/x86/include/asm/mpx.h~mpx-new-entry-to-addr-helper arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~mpx-new-entry-to-addr-helper	2014-12-12 11:11:40.986033200 -0800
+++ b/arch/x86/include/asm/mpx.h	2014-12-12 11:11:40.991033426 -0800
@@ -45,7 +45,6 @@
 #define MPX_BNDSTA_TAIL		2
 #define MPX_BNDCFG_TAIL		12
 #define MPX_BNDSTA_ADDR_MASK	(~((1UL<<MPX_BNDSTA_TAIL)-1))
-#define MPX_BT_ADDR_MASK	(~((1UL<<MPX_BD_ENTRY_TAIL)-1))
 
 #define MPX_BNDCFG_ADDR_MASK	(~((1UL<<MPX_BNDCFG_TAIL)-1))
 #define MPX_BNDSTA_ERROR_CODE	0x3
diff -puN arch/x86/mm/mpx.c~mpx-new-entry-to-addr-helper arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~mpx-new-entry-to-addr-helper	2014-12-12 11:11:40.988033290 -0800
+++ b/arch/x86/mm/mpx.c	2014-12-12 11:11:40.992033471 -0800
@@ -567,29 +567,55 @@ static int mpx_resolve_fault(long __user
 	return 0;
 }
 
+static unsigned long mpx_bd_entry_to_bt_addr(struct mm_struct *mm,
+		unsigned long bd_entry)
+{
+	unsigned long bt_addr = bd_entry;
+	int align_to_bytes;
+	/*
+	 * Bit 0 in a bt_entry is always the valid bit.
+	 */
+	bt_addr &= ~MPX_BD_ENTRY_VALID_FLAG;
+	/*
+	 * Tables are naturally aligned at 8-byte boundaries
+	 * on 64-bit and 4-byte boundaries on 32-bit.  The
+	 * documentation makes it appear that the low bits
+	 * are ignored by the hardware, so we do the same.
+	 */
+	if (is_64bit_mm(mm))
+		align_to_bytes = 8;
+	else
+		align_to_bytes = 4;
+	bt_addr &= ~(align_to_bytes-1);
+	return bt_addr;
+}
+
 /*
  * Get the base of bounds tables pointed by specific bounds
  * directory entry.
  */
 static int get_bt_addr(struct mm_struct *mm,
-			long __user *bd_entry, unsigned long *bt_addr)
+			long __user *bd_entry_ptr,
+			unsigned long *bt_addr_result)
 {
 	int ret;
 	int valid_bit;
+	unsigned long bd_entry;
+	unsigned long bt_addr;
 
-	if (!access_ok(VERIFY_READ, (bd_entry), sizeof(*bd_entry)))
+	if (!access_ok(VERIFY_READ, (bd_entry_ptr), sizeof(*bd_entry_ptr)))
 		return -EFAULT;
 
 	while (1) {
 		int need_write = 0;
 
 		pagefault_disable();
-		ret = get_user(*bt_addr, bd_entry);
+		ret = get_user(bd_entry, bd_entry_ptr);
 		pagefault_enable();
 		if (!ret)
 			break;
 		if (ret == -EFAULT)
-			ret = mpx_resolve_fault(bd_entry, need_write);
+			ret = mpx_resolve_fault(bd_entry_ptr, need_write);
 		/*
 		 * If we could not resolve the fault, consider it
 		 * userspace's fault and error out.
@@ -598,8 +624,8 @@ static int get_bt_addr(struct mm_struct
 			return ret;
 	}
 
-	valid_bit = *bt_addr & MPX_BD_ENTRY_VALID_FLAG;
-	*bt_addr &= MPX_BT_ADDR_MASK;
+	valid_bit = bd_entry & MPX_BD_ENTRY_VALID_FLAG;
+	bt_addr = mpx_bd_entry_to_bt_addr(mm, bd_entry);
 
 	/*
 	 * When the kernel is managing bounds tables, a bounds directory
@@ -608,7 +634,7 @@ static int get_bt_addr(struct mm_struct
 	 * data in the address field, we know something is wrong. This
 	 * -EINVAL return will cause a SIGSEGV.
 	 */
-	if (!valid_bit && *bt_addr)
+	if (!valid_bit && bt_addr)
 		return -EINVAL;
 	/*
 	 * Do we have an completely zeroed bt entry?  That is OK.  It
@@ -619,6 +645,7 @@ static int get_bt_addr(struct mm_struct
 	if (!valid_bit)
 		return -ENOENT;
 
+	*bt_addr_result = bt_addr;
 	return 0;
 }
 
_

[RFC][PATCH 7/8] x86, mpx: do 32-bit-only cmpxchg for 32-bit apps

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

user_atomic_cmpxchg_inatomic() actually looks at sizeof(*ptr) to
figure out how many bytes to copy.  If we run it on a 64-bit
kernel with a 64-bit pointer, it will copy a 64-bit bounds
directory entry.  That's fine, except when we have 32-bit
programs with 32-bit bounds directory entries and we only *want*
32-bits.

This patch breaks the cmpxchg operation out in to its own
function and performs the 32-bit type swizzling in there.

Note, the "64-bit" version of this code _would_ work on a
32-bit-only kernel.  The issue this patch addresses is only for
when the kernel's 'long' is mismatched from the size of the
bounds directory entry of the process we are working on.

The new helper modifies 'actual_old_val' or returns an error.
But gcc doesn't know this, so it warns about 'actual_old_val'
being unused.  Shut it up with an uninitialized_var().

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/mm/mpx.c |   41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff -puN arch/x86/mm/mpx.c~mpx-variable-sized-userspace-pokes arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~mpx-variable-sized-userspace-pokes	2014-12-12 11:11:41.385051197 -0800
+++ b/arch/x86/mm/mpx.c	2014-12-12 11:11:41.388051332 -0800
@@ -411,6 +411,35 @@ int mpx_disable_management(struct task_s
 	return 0;
 }
 
+static int mpx_cmpxchg_bd_entry(struct mm_struct *mm,
+		unsigned long *actual_old_val_ptr, long __user *bd_entry_addr,
+		unsigned long expected_old_val, unsigned long new_bd_entry)
+{
+	int ret;
+	/*
+	 * user_atomic_cmpxchg_inatomic() actually uses sizeof()
+	 * the pointer thatt we pass to it to figure out how much
+	 * data to cmpxchg.  We have to be careful here not to
+	 * pass a pointer to a 64-bit data type when we only want
+	 * a 32-bit copy.
+	 */
+	if (is_64bit_mm(mm)) {
+		ret = user_atomic_cmpxchg_inatomic(actual_old_val_ptr,
+				bd_entry_addr, expected_old_val, new_bd_entry);
+	} else {
+		u32 uninitialized_var(actual_old_val_32);
+		u32 expected_old_val_32 = expected_old_val;
+		u32 new_bd_entry_32 = new_bd_entry;
+		u32 __user *bd_entry_32 = (u32 __user *)bd_entry_addr;
+		ret = user_atomic_cmpxchg_inatomic(&actual_old_val_32,
+				bd_entry_32, expected_old_val_32,
+				new_bd_entry_32);
+		if (!ret)
+			*actual_old_val_ptr = actual_old_val_32;
+	}
+	return ret;
+}
+
 /*
  * With 32-bit mode, MPX_BT_SIZE_BYTES is 4MB, and the size of each
  * bounds table is 16KB. With 64-bit mode, MPX_BT_SIZE_BYTES is 2GB,
@@ -418,6 +447,7 @@ int mpx_disable_management(struct task_s
  */
 static int allocate_bt(long __user *bd_entry)
 {
+	struct mm_struct *mm = current->mm;
 	unsigned long expected_old_val = 0;
 	unsigned long actual_old_val = 0;
 	unsigned long bt_addr;
@@ -447,8 +477,8 @@ static int allocate_bt(long __user *bd_e
 	 * mmap_sem at this point, unlike some of the other part
 	 * of the MPX code that have to pagefault_disable().
 	 */
-	ret = user_atomic_cmpxchg_inatomic(&actual_old_val, bd_entry,
-					   expected_old_val, bd_new_entry);
+	ret = mpx_cmpxchg_bd_entry(mm, &actual_old_val,	bd_entry,
+				   expected_old_val, bd_new_entry);
 	if (ret)
 		goto out_unmap;
 
@@ -700,15 +730,16 @@ static int unmap_single_bt(struct mm_str
 		long __user *bd_entry, unsigned long bt_addr)
 {
 	unsigned long expected_old_val = bt_addr | MPX_BD_ENTRY_VALID_FLAG;
-	unsigned long actual_old_val = 0;
+	unsigned long uninitialized_var(actual_old_val);
 	int ret;
 
 	while (1) {
 		int need_write = 1;
+		unsigned long cleared_bd_entry = 0;
 
 		pagefault_disable();
-		ret = user_atomic_cmpxchg_inatomic(&actual_old_val, bd_entry,
-						   expected_old_val, 0);
+		ret = mpx_cmpxchg_bd_entry(mm, &actual_old_val,
+				bd_entry, bt_addr, cleared_bd_entry);
 		pagefault_enable();
 		if (!ret)
 			break;
_

[RFC][PATCH 8/8] x86, mpx: support 32bit binaries on 64bit kernel

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Right now, the kernel can only switch between 64-bit and 32-bit
binaries at compile time. This patch adds support for 32-bit
binaries on 64-bit kernels when we support ia32 emulation.

We essentially choose which set of table sizes to use when doing
arithmetic for the bounds table calculations.

This also uses a different approach for calculating the table
indexes than before.  I think the new one makes it much more
clear what is going on, and allows us to share more code between
the 32 and 64-bit cases.

Based-on-patch-by: Qiaowei Ren <qiaowei.ren@intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/mpx.h |   68 +++++++++----------
 b/arch/x86/mm/mpx.c          |  150 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 163 insertions(+), 55 deletions(-)

diff -puN arch/x86/include/asm/mpx.h~0002-x86-mpx-support-32bit-binaries-on-64bit-kernel arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~0002-x86-mpx-support-32bit-binaries-on-64bit-kernel	2014-12-12 11:11:41.763068246 -0800
+++ b/arch/x86/include/asm/mpx.h	2014-12-12 11:11:41.768068471 -0800
@@ -13,49 +13,49 @@
 #define MPX_BNDCFG_ENABLE_FLAG	0x1
 #define MPX_BD_ENTRY_VALID_FLAG	0x1
 
-#ifdef CONFIG_X86_64
-
-/* upper 28 bits [47:20] of the virtual address in 64-bit used to
- * index into bounds directory (BD).
+/*
+ * The upper 28 bits [47:20] of the virtual address in 64-bit
+ * are used to index into bounds directory (BD).
+ *
+ * The directory is 2G (2^31) in size, and with 8-byte entries
+ * it has 2^28 entries.
  */
-#define MPX_BD_ENTRY_OFFSET	28
-#define MPX_BD_ENTRY_SHIFT	3
-/* bits [19:3] of the virtual address in 64-bit used to index into
- * bounds table (BT).
+#define MPX_BD_SIZE_BYTES_64	(1UL<<31)
+/* An entry is a long, so 8 bytes and a shift of 3 */
+#define MPX_BD_ENTRY_BYTES_64	8
+#define MPX_BD_NR_ENTRIES_64	(MPX_BD_SIZE_BYTES_64/MPX_BD_ENTRY_BYTES_64)
+
+/*
+ * The 32-bit directory is 4MB (2^22) in size, and with 4-byte
+ * entries it has 2^20 entries.
  */
-#define MPX_BT_ENTRY_OFFSET	17
-#define MPX_BT_ENTRY_SHIFT	5
-#define MPX_IGN_BITS		3
-#define MPX_BD_ENTRY_TAIL	3
-
-#else
-
-#define MPX_BD_ENTRY_OFFSET	20
-#define MPX_BD_ENTRY_SHIFT	2
-#define MPX_BT_ENTRY_OFFSET	10
-#define MPX_BT_ENTRY_SHIFT	4
-#define MPX_IGN_BITS		2
-#define MPX_BD_ENTRY_TAIL	2
-
-#endif
-
-#define MPX_BD_SIZE_BYTES (1UL<<(MPX_BD_ENTRY_OFFSET+MPX_BD_ENTRY_SHIFT))
-#define MPX_BT_SIZE_BYTES (1UL<<(MPX_BT_ENTRY_OFFSET+MPX_BT_ENTRY_SHIFT))
+#define MPX_BD_SIZE_BYTES_32	(1UL<<22)
+/* An entry is a long, so 4 bytes and a shift of 2 */
+#define MPX_BD_ENTRY_BYTES_32	4
+#define MPX_BD_NR_ENTRIES_32	(MPX_BD_SIZE_BYTES_32/MPX_BD_ENTRY_BYTES_32)
+
+/*
+ * A 64-bit table is 4MB total in size, and an entry is
+ * 4 64-bit pointers in size.
+ */
+#define MPX_BT_SIZE_BYTES_64	(1UL<<22)
+#define MPX_BT_ENTRY_BYTES_64	32
+#define MPX_BT_NR_ENTRIES_64	(MPX_BD_SIZE_BYTES_64/MPX_BD_ENTRY_BYTES_64)
+
+/*
+ * A 32-bit table is 16kB total in size, and an entry is
+ * 4 32-bit pointers in size.
+ */
+#define MPX_BT_SIZE_BYTES_32	(1UL<<14)
+#define MPX_BT_ENTRY_BYTES_32	16
+#define MPX_BT_NR_ENTRIES_32	(MPX_BD_SIZE_BYTES_32/MPX_BD_ENTRY_BYTES_32)
 
 #define MPX_BNDSTA_TAIL		2
 #define MPX_BNDCFG_TAIL		12
 #define MPX_BNDSTA_ADDR_MASK	(~((1UL<<MPX_BNDSTA_TAIL)-1))
-
 #define MPX_BNDCFG_ADDR_MASK	(~((1UL<<MPX_BNDCFG_TAIL)-1))
 #define MPX_BNDSTA_ERROR_CODE	0x3
 
-#define MPX_BD_ENTRY_MASK	((1<<MPX_BD_ENTRY_OFFSET)-1)
-#define MPX_BT_ENTRY_MASK	((1<<MPX_BT_ENTRY_OFFSET)-1)
-#define MPX_GET_BD_ENTRY_OFFSET(addr)	((((addr)>>(MPX_BT_ENTRY_OFFSET+ \
-		MPX_IGN_BITS)) & MPX_BD_ENTRY_MASK) << MPX_BD_ENTRY_SHIFT)
-#define MPX_GET_BT_ENTRY_OFFSET(addr)	((((addr)>>MPX_IGN_BITS) & \
-		MPX_BT_ENTRY_MASK) << MPX_BT_ENTRY_SHIFT)
-
 #ifdef CONFIG_X86_INTEL_MPX
 siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
 				struct xsave_struct *xsave_buf);
diff -puN arch/x86/mm/mpx.c~0002-x86-mpx-support-32bit-binaries-on-64bit-kernel arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~0002-x86-mpx-support-32bit-binaries-on-64bit-kernel	2014-12-12 11:11:41.764068291 -0800
+++ b/arch/x86/mm/mpx.c	2014-12-12 11:11:41.768068471 -0800
@@ -32,6 +32,22 @@ static int is_mpx_vma(struct vm_area_str
 	return (vma->vm_ops == &mpx_vma_ops);
 }
 
+static inline unsigned long mpx_bd_size_bytes(struct mm_struct *mm)
+{
+	if (is_64bit_mm(mm))
+		return MPX_BD_SIZE_BYTES_64;
+	else
+		return MPX_BD_SIZE_BYTES_32;
+}
+
+static inline unsigned long mpx_bt_size_bytes(struct mm_struct *mm)
+{
+	if (is_64bit_mm(mm))
+		return MPX_BT_SIZE_BYTES_64;
+	else
+		return MPX_BT_SIZE_BYTES_32;
+}
+
 /*
  * This is really a simplified "vm_mmap". it only handles MPX
  * bounds tables (the bounds directory is user-allocated).
@@ -48,7 +64,7 @@ static unsigned long mpx_mmap(unsigned l
 	struct vm_area_struct *vma;
 
 	/* Only bounds table can be allocated here */
-	if (len != MPX_BT_SIZE_BYTES)
+	if (len != mpx_bt_size_bytes(mm))
 		return -EINVAL;
 
 	down_write(&mm->mmap_sem);
@@ -441,13 +457,12 @@ static int mpx_cmpxchg_bd_entry(struct m
 }
 
 /*
- * With 32-bit mode, MPX_BT_SIZE_BYTES is 4MB, and the size of each
- * bounds table is 16KB. With 64-bit mode, MPX_BT_SIZE_BYTES is 2GB,
+ * With 32-bit mode, a bounds directory is 4MB, and the size of each
+ * bounds table is 16KB. With 64-bit mode, a bounds directory is 2GB,
  * and the size of each bounds table is 4MB.
  */
-static int allocate_bt(long __user *bd_entry)
+static int allocate_bt(struct mm_struct *mm, long __user *bd_entry)
 {
-	struct mm_struct *mm = current->mm;
 	unsigned long expected_old_val = 0;
 	unsigned long actual_old_val = 0;
 	unsigned long bt_addr;
@@ -458,7 +473,7 @@ static int allocate_bt(long __user *bd_e
 	 * Carve the virtual space out of userspace for the new
 	 * bounds table:
 	 */
-	bt_addr = mpx_mmap(MPX_BT_SIZE_BYTES);
+	bt_addr = mpx_mmap(mpx_bt_size_bytes(mm));
 	if (IS_ERR((void *)bt_addr))
 		return PTR_ERR((void *)bt_addr);
 	/*
@@ -508,7 +523,7 @@ static int allocate_bt(long __user *bd_e
 	}
 	return 0;
 out_unmap:
-	vm_munmap(bt_addr, MPX_BT_SIZE_BYTES);
+	vm_munmap(bt_addr, mpx_bt_size_bytes(mm));
 	return ret;
 }
 
@@ -527,6 +542,7 @@ static int do_mpx_bt_fault(struct xsave_
 {
 	unsigned long bd_entry, bd_base;
 	struct bndcsr *bndcsr;
+	struct mm_struct *mm = current->mm;
 
 	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
 	if (!bndcsr)
@@ -545,10 +561,10 @@ static int do_mpx_bt_fault(struct xsave_
 	 * the directory is.
 	 */
 	if ((bd_entry < bd_base) ||
-	    (bd_entry >= bd_base + MPX_BD_SIZE_BYTES))
+	    (bd_entry >= bd_base + mpx_bd_size_bytes(mm)))
 		return -EINVAL;
 
-	return allocate_bt((long __user *)bd_entry);
+	return allocate_bt(mm, (long __user *)bd_entry);
 }
 
 int mpx_handle_bd_fault(struct xsave_struct *xsave_buf)
@@ -779,7 +795,95 @@ static int unmap_single_bt(struct mm_str
 	 * avoid recursion, do_munmap() will check whether it comes
 	 * from one bounds table through VM_MPX flag.
 	 */
-	return do_munmap(mm, bt_addr, MPX_BT_SIZE_BYTES);
+	return do_munmap(mm, bt_addr, mpx_bt_size_bytes(mm));
+}
+
+/*
+ * Take a virtual address and turns it in to the offset in bytes
+ * inside of the bounds table where the bounds table entry
+ * controlling 'addr' can be found.
+ */
+static unsigned long mpx_get_bt_entry_offset_bytes(struct mm_struct *mm,
+		unsigned long addr)
+{
+	unsigned long bt_entry_size_bytes;
+	unsigned long bt_table_nr_entries;
+	unsigned long offset = addr;
+
+	if (is_64bit_mm(mm)) {
+		/* Bottom 3 bits are ignored on 64-bit */
+		offset >>= 3;
+		bt_entry_size_bytes = MPX_BT_ENTRY_BYTES_64;
+		bt_table_nr_entries = MPX_BT_NR_ENTRIES_64;
+	} else {
+		/* Bottom 2 bits are ignored on 32-bit */
+		offset >>= 2;
+		bt_entry_size_bytes = MPX_BT_ENTRY_BYTES_32;
+		bt_table_nr_entries = MPX_BT_NR_ENTRIES_32;
+	}
+	/*
+	 * We know the size of the table in to which we are
+	 * indexing, and we have eliminated all the low bits
+	 * which are ignored for indexing.
+	 *
+	 * Mask out all the high bits which we do not need
+	 * to index in to the table.
+	 */
+	offset &= (bt_table_nr_entries-1);
+	/*
+	 * We now have an entry offset in terms of *entries* in
+	 * the table.  We need to scale it back up to bytes.
+	 */
+	offset *= bt_entry_size_bytes;
+	return offset;
+}
+
+static noinline unsigned long mpx_get_bd_entry_offset(struct mm_struct *mm,
+		unsigned long addr)
+{
+	/*
+	 * Total size of the process's virtual address space
+	 * Use a u64 because 4GB (for 32-bit) won't fit in a long.
+	 */
+	u64 vaddr_space_size;
+	/*
+	 * How much virtual address space does a single bounds
+	 * directory entry cover?
+	 */
+	unsigned long bd_entry_virt_space;
+
+	/*
+	 * There are several ways to derive the bd offsets.  We
+	 * use the following approach here:
+	 * 1. We know the size of the virtual address space
+	 * 2. We know the number of entries in a bounds table
+	 * 3. We know that each entry covers a fixed amount of
+	 *    virtual address space.
+	 * So, we can just divide the virtual address by the
+	 * number of entries to figure out which entry "controls"
+	 * the given virtual address.
+	 */
+	if (is_64bit_mm(mm)) {
+	      	vaddr_space_size = 1ULL << __VIRTUAL_MASK_SHIFT;
+		bd_entry_virt_space = vaddr_space_size / MPX_BD_NR_ENTRIES_64;
+		/*
+		 * __VIRTUAL_MASK takes the 64-bit addressing hole
+		 * in to accout.  This is a noop on 32-bit.
+		 */
+		addr &= __VIRTUAL_MASK;
+		return addr / bd_entry_virt_space;
+	} else {
+	      	vaddr_space_size = (1ULL << 32);
+		bd_entry_virt_space = vaddr_space_size / MPX_BD_NR_ENTRIES_32;
+		return addr / bd_entry_virt_space;
+	}
+	/*
+	 * The two return calls above are exact copies.  If we
+	 * pull out a single copy and put it in here, gcc won't
+	 * realize that we're doing a power-of-2 divide and use
+	 * shifts.  It uses a real divide.  If we put them up
+	 * there, it manages to figure it out (gcc 4.8.3).
+	 */
 }
 
 /*
@@ -793,6 +897,7 @@ static int unmap_shared_bt(struct mm_str
 		unsigned long end, bool prev_shared, bool next_shared)
 {
 	unsigned long bt_addr;
+	unsigned long start_off, end_off;
 	int ret;
 
 	ret = get_bt_addr(mm, bd_entry, &bt_addr);
@@ -804,17 +909,20 @@ static int unmap_shared_bt(struct mm_str
 	if (ret)
 		return ret;
 
+	start_off = mpx_get_bt_entry_offset_bytes(mm, start);
+	end_off   = mpx_get_bt_entry_offset_bytes(mm, end);
+
 	if (prev_shared && next_shared)
 		ret = zap_bt_entries(mm, bt_addr,
-				bt_addr+MPX_GET_BT_ENTRY_OFFSET(start),
-				bt_addr+MPX_GET_BT_ENTRY_OFFSET(end));
+				bt_addr+start_off,
+				bt_addr+end_off);
 	else if (prev_shared)
 		ret = zap_bt_entries(mm, bt_addr,
-				bt_addr+MPX_GET_BT_ENTRY_OFFSET(start),
-				bt_addr+MPX_BT_SIZE_BYTES);
+				bt_addr + start_off,
+				bt_addr + mpx_bt_size_bytes(mm));
 	else if (next_shared)
 		ret = zap_bt_entries(mm, bt_addr, bt_addr,
-				bt_addr+MPX_GET_BT_ENTRY_OFFSET(end));
+				bt_addr+end_off);
 	else
 		ret = unmap_single_bt(mm, bd_entry, bt_addr);
 
@@ -835,8 +943,8 @@ static int unmap_edge_bts(struct mm_stru
 	struct vm_area_struct *prev, *next;
 	bool prev_shared = false, next_shared = false;
 
-	bde_start = mm->bd_addr + MPX_GET_BD_ENTRY_OFFSET(start);
-	bde_end = mm->bd_addr + MPX_GET_BD_ENTRY_OFFSET(end-1);
+	bde_start = mm->bd_addr + mpx_get_bd_entry_offset(mm, start);
+	bde_end   = mm->bd_addr + mpx_get_bd_entry_offset(mm, end-1);
 
 	/*
 	 * Check whether bde_start and bde_end are shared with adjacent
@@ -848,10 +956,10 @@ static int unmap_edge_bts(struct mm_stru
 	 * in to 'next'.
 	 */
 	next = find_vma_prev(mm, start, &prev);
-	if (prev && (mm->bd_addr + MPX_GET_BD_ENTRY_OFFSET(prev->vm_end-1))
+	if (prev && (mm->bd_addr + mpx_get_bd_entry_offset(mm, prev->vm_end-1))
 			== bde_start)
 		prev_shared = true;
-	if (next && (mm->bd_addr + MPX_GET_BD_ENTRY_OFFSET(next->vm_start))
+	if (next && (mm->bd_addr + mpx_get_bd_entry_offset(mm, next->vm_start))
 			== bde_end)
 		next_shared = true;
 
@@ -916,8 +1024,8 @@ static int mpx_unmap_tables(struct mm_st
 	 *   1. fully covered
 	 *   2. not at the edges of the mapping, even if full aligned
 	 */
-	bde_start = mm->bd_addr + MPX_GET_BD_ENTRY_OFFSET(start);
-	bde_end = mm->bd_addr + MPX_GET_BD_ENTRY_OFFSET(end-1);
+	bde_start = mm->bd_addr + mpx_get_bd_entry_offset(mm, start);
+	bde_end   = mm->bd_addr + mpx_get_bd_entry_offset(mm, end-1);
 	for (bd_entry = bde_start + 1; bd_entry < bde_end; bd_entry++) {
 		ret = get_bt_addr(mm, bd_entry, &bt_addr);
 		switch (ret) {
_

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Andy Lutomirski]

On 12/12/2014 11:12 AM, Dave Hansen wrote:
> This is 3.20 material.  I'm hoping to get some comments early
> in case folks have some issues with the way it's being done.
>
> --
>
> The MPX hardware structures differ in layout in 32 and 64-bit
> mode.  A 32-bit binary running on a 64-bit kernel needs the
> 32-bit structures, so we need code which switches between
> the two modes.

The OS community figured out years ago that you aren't supposed to do 
this.  Did everyone forget to tell the hardware people?

:(

--Andy

>
> The first patch is just a documentation update.  The next 5
> patches are really just prepwork.  The meat is in the last 3.
>
> 	x86: make is_64bit_mm() widely available
> 	x86: make __VIRTUAL_MASK safe to use on 32 bit
> 	x86, mpx: we do not allocate the bounds directory
> 	x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK
> 	x86, mpx: Add temporary variable to reduce masking
> 	x86, mpx: new directory entry to addr helper
> 	x86, mpx: do 32-bit-only cmpxchg for 32-bit apps
> 	x86, mpx: support 32bit binaries on 64bit kernel
>
>   Documentation/x86/intel_mpx.txt    |   12 +++-
>   arch/x86/include/asm/mmu_context.h |   13 +++++
>   arch/x86/include/asm/mpx.h         |    2
>   arch/x86/include/asm/page_types.h  |    8 +++
>   arch/x86/kernel/uprobes.c          |   10 ---
>   arch/x86/mm/mpx.c                  |   93 ++++++++++++++++++++++++++++++-------
>   6 files changed, 107 insertions(+), 31 deletions(-)
>

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Dave Hansen]

On 12/12/2014 12:22 PM, Andy Lutomirski wrote:
> On 12/12/2014 11:12 AM, Dave Hansen wrote:
>> This is 3.20 material.  I'm hoping to get some comments early
>> in case folks have some issues with the way it's being done.
>>
>> The MPX hardware structures differ in layout in 32 and 64-bit
>> mode.  A 32-bit binary running on a 64-bit kernel needs the
>> 32-bit structures, so we need code which switches between
>> the two modes.
> 
> The OS community figured out years ago that you aren't supposed to do
> this.  Did everyone forget to tell the hardware people?

What is your concern with it, exactly?

You want the same size structures with the same format for 32-bit and
64-bit modes?

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Andy Lutomirski]

On Fri, Dec 12, 2014 at 12:27 PM, Dave Hansen <dave@sr71.net> wrote:
> On 12/12/2014 12:22 PM, Andy Lutomirski wrote:
>> On 12/12/2014 11:12 AM, Dave Hansen wrote:
>>> This is 3.20 material.  I'm hoping to get some comments early
>>> in case folks have some issues with the way it's being done.
>>>
>>> The MPX hardware structures differ in layout in 32 and 64-bit
>>> mode.  A 32-bit binary running on a 64-bit kernel needs the
>>> 32-bit structures, so we need code which switches between
>>> the two modes.
>>
>> The OS community figured out years ago that you aren't supposed to do
>> this.  Did everyone forget to tell the hardware people?
>
> What is your concern with it, exactly?
>
> You want the same size structures with the same format for 32-bit and
> 64-bit modes?

Yes.  Especially because programs can switch between 32-bit and 64-bit
mode entirely in userspace.  I don't know whether any do in practice,
but programs *can*.

Or better yet: Intel could have skipped supporting it at all in 32-bit
mode.  Isn't mpx somewhat of an address space hog anyway?

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Dave Hansen]

On 12/12/2014 12:48 PM, Andy Lutomirski wrote:
> On Fri, Dec 12, 2014 at 12:27 PM, Dave Hansen <dave@sr71.net> wrote:
>> You want the same size structures with the same format for 32-bit and
>> 64-bit modes?
> 
> Yes.  Especially because programs can switch between 32-bit and 64-bit
> mode entirely in userspace.  I don't know whether any do in practice,
> but programs *can*.

So, you want a 2GB of the 32-bit address space dedicated to a bounds
directory, and half of the space for the bounds tables to be simply
zero-filled unused address bits?  That seems, um, a bit unreasonable.

> Or better yet: Intel could have skipped supporting it at all in 32-bit
> mode.  

So, we should not have this security feature for 32-bit apps... because
it costs us 50 lines of code in the kernel to support?  Did you look at
the diffstat?

> Isn't mpx somewhat of an address space hog anyway?

Yes, it will be troublesome for 32-bit apps that are already bumping up
against the virtual address space size to support it.  But, really, how
many of those *are* there these days?

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Andy Lutomirski]

n Fri, Dec 12, 2014 at 1:41 PM, Dave Hansen <dave@sr71.net> wrote:
> On 12/12/2014 12:48 PM, Andy Lutomirski wrote:
>> On Fri, Dec 12, 2014 at 12:27 PM, Dave Hansen <dave@sr71.net> wrote:
>>> You want the same size structures with the same format for 32-bit and
>>> 64-bit modes?
>>
>> Yes.  Especially because programs can switch between 32-bit and 64-bit
>> mode entirely in userspace.  I don't know whether any do in practice,
>> but programs *can*.
>
> So, you want a 2GB of the 32-bit address space dedicated to a bounds
> directory, and half of the space for the bounds tables to be simply
> zero-filled unused address bits?  That seems, um, a bit unreasonable.

Fair enough.

>
>> Or better yet: Intel could have skipped supporting it at all in 32-bit
>> mode.
>
> So, we should not have this security feature for 32-bit apps... because
> it costs us 50 lines of code in the kernel to support?  Did you look at
> the diffstat?
>
>> Isn't mpx somewhat of an address space hog anyway?
>
> Yes, it will be troublesome for 32-bit apps that are already bumping up
> against the virtual address space size to support it.  But, really, how
> many of those *are* there these days?

I wonder how many 32-bit apps will end up using mpx regardless.

Anyway, do your patches handle the case where a 32-bit app maliciously
executes a 64-bit mpx insn with a very large address?  I think it's
okay, but I might have missed something.

--Andy

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Dave Hansen]

On 12/12/2014 03:04 PM, Andy Lutomirski wrote:
> Anyway, do your patches handle the case where a 32-bit app maliciously
> executes a 64-bit mpx insn with a very large address?  I think it's
> okay, but I might have missed something.

You mean in the instruction decoder?  I haven't tried that case
explicitly, but I did do a substantial amount of testing throwing random
instruction streams at the decoder to make sure it never fell over.
(Well, mostly random, I made sure to throw the MPX opcodes in there a
bunch so it would get much deeper in to the decoder).

It's not about the instruction size, it's about the mode the CPU is in.
If a 32-bit app manages to switch over to 64-bit mode and doesn't tell
the kernel (TIF_IA32 remains set), then we'll treat it as a 32-bit
instruction.

The kernel might end up going and looking for the bounds tables in some
funky places if the kernel and the hardware disagree about 32 vs. 64-bit
modes, but it's not going to do any harm since we treat all of the data
we get from MPX (instruction decoding, register contents, bounds table
contents, etc...) as completely untrusted.

It's a nice, paranoid thing to ask and I'm glad you brought it up
because I hadn't thought about it, but I don't think any harm can come
of it.

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Andy Lutomirski]

On Fri, Dec 12, 2014 at 3:16 PM, Dave Hansen <dave@sr71.net> wrote:
> On 12/12/2014 03:04 PM, Andy Lutomirski wrote:
>> Anyway, do your patches handle the case where a 32-bit app maliciously
>> executes a 64-bit mpx insn with a very large address?  I think it's
>> okay, but I might have missed something.
>
> You mean in the instruction decoder?  I haven't tried that case
> explicitly, but I did do a substantial amount of testing throwing random
> instruction streams at the decoder to make sure it never fell over.
> (Well, mostly random, I made sure to throw the MPX opcodes in there a
> bunch so it would get much deeper in to the decoder).
>
> It's not about the instruction size, it's about the mode the CPU is in.
> If a 32-bit app manages to switch over to 64-bit mode and doesn't tell
> the kernel (TIF_IA32 remains set), then we'll treat it as a 32-bit
> instruction.

The insn decoder should probably use user_64bit_mode, not TIF_IA32.
It's actually quite easy to far jump/call/ret or sigreturn to a
different bitness.

>
> The kernel might end up going and looking for the bounds tables in some
> funky places if the kernel and the hardware disagree about 32 vs. 64-bit
> modes, but it's not going to do any harm since we treat all of the data
> we get from MPX (instruction decoding, register contents, bounds table
> contents, etc...) as completely untrusted.
>
> It's a nice, paranoid thing to ask and I'm glad you brought it up
> because I hadn't thought about it, but I don't think any harm can come
> of it.

Paranoia is fun!

The only thing I'd really be worried about is if the code that turns
va into bounds table offset generates some absurdly large offset as a
result and causes a problem.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Dave Hansen]

On 12/12/2014 04:11 PM, Andy Lutomirski wrote:
> On Fri, Dec 12, 2014 at 3:16 PM, Dave Hansen <dave@sr71.net> wrote:
>> On 12/12/2014 03:04 PM, Andy Lutomirski wrote:
>>> Anyway, do your patches handle the case where a 32-bit app maliciously
>>> executes a 64-bit mpx insn with a very large address?  I think it's
>>> okay, but I might have missed something.
>>
>> You mean in the instruction decoder?  I haven't tried that case
>> explicitly, but I did do a substantial amount of testing throwing random
>> instruction streams at the decoder to make sure it never fell over.
>> (Well, mostly random, I made sure to throw the MPX opcodes in there a
>> bunch so it would get much deeper in to the decoder).
>>
>> It's not about the instruction size, it's about the mode the CPU is in.
>> If a 32-bit app manages to switch over to 64-bit mode and doesn't tell
>> the kernel (TIF_IA32 remains set), then we'll treat it as a 32-bit
>> instruction.
> 
> The insn decoder should probably use user_64bit_mode, not TIF_IA32.
> It's actually quite easy to far jump/call/ret or sigreturn to a
> different bitness.

There are number of examples of this in the kernel today:

#ifdef CONFIG_X86_64
                is_64bit = kernel_ip(to) || !test_thread_flag(TIF_IA32);
#endif
                insn_init(&insn, kaddr, size, is_64bit);

Are you saying that those need to get fixed up?

>> The kernel might end up going and looking for the bounds tables in some
>> funky places if the kernel and the hardware disagree about 32 vs. 64-bit
>> modes, but it's not going to do any harm since we treat all of the data
>> we get from MPX (instruction decoding, register contents, bounds table
>> contents, etc...) as completely untrusted.
>>
>> It's a nice, paranoid thing to ask and I'm glad you brought it up
>> because I hadn't thought about it, but I don't think any harm can come
>> of it.
> 
> Paranoia is fun!
> 
> The only thing I'd really be worried about is if the code that turns
> va into bounds table offset generates some absurdly large offset as a
> result and causes a problem.

The instructions that get decoded have *NOTHING* to do with the mode
we're running in.  By the time we take a bounds fault and copy the
instruction in from the instruction pointer, we have absolutely no idea
what was actually being executed, no matter what mode we are running in.

I believe the instruction decoder already happily handles this.

Furthermore, we don't even *USE* the result of the instruction decode in
the kernel.  We toss it in to the siginfo and hand it out to userspace.

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Andy Lutomirski]

On Fri, Dec 12, 2014 at 4:23 PM, Dave Hansen <dave@sr71.net> wrote:
> On 12/12/2014 04:11 PM, Andy Lutomirski wrote:
>> On Fri, Dec 12, 2014 at 3:16 PM, Dave Hansen <dave@sr71.net> wrote:
>>> On 12/12/2014 03:04 PM, Andy Lutomirski wrote:
>>>> Anyway, do your patches handle the case where a 32-bit app maliciously
>>>> executes a 64-bit mpx insn with a very large address?  I think it's
>>>> okay, but I might have missed something.
>>>
>>> You mean in the instruction decoder?  I haven't tried that case
>>> explicitly, but I did do a substantial amount of testing throwing random
>>> instruction streams at the decoder to make sure it never fell over.
>>> (Well, mostly random, I made sure to throw the MPX opcodes in there a
>>> bunch so it would get much deeper in to the decoder).
>>>
>>> It's not about the instruction size, it's about the mode the CPU is in.
>>> If a 32-bit app manages to switch over to 64-bit mode and doesn't tell
>>> the kernel (TIF_IA32 remains set), then we'll treat it as a 32-bit
>>> instruction.
>>
>> The insn decoder should probably use user_64bit_mode, not TIF_IA32.
>> It's actually quite easy to far jump/call/ret or sigreturn to a
>> different bitness.
>
> There are number of examples of this in the kernel today:
>
> #ifdef CONFIG_X86_64
>                 is_64bit = kernel_ip(to) || !test_thread_flag(TIF_IA32);
> #endif
>                 insn_init(&insn, kaddr, size, is_64bit);
>
> Are you saying that those need to get fixed up?
>

Yes, although so far it looks like the only real danger with them is
that userspace could shoot itself in the foot.

>>> The kernel might end up going and looking for the bounds tables in some
>>> funky places if the kernel and the hardware disagree about 32 vs. 64-bit
>>> modes, but it's not going to do any harm since we treat all of the data
>>> we get from MPX (instruction decoding, register contents, bounds table
>>> contents, etc...) as completely untrusted.
>>>
>>> It's a nice, paranoid thing to ask and I'm glad you brought it up
>>> because I hadn't thought about it, but I don't think any harm can come
>>> of it.
>>
>> Paranoia is fun!
>>
>> The only thing I'd really be worried about is if the code that turns
>> va into bounds table offset generates some absurdly large offset as a
>> result and causes a problem.
>
> The instructions that get decoded have *NOTHING* to do with the mode
> we're running in.  By the time we take a bounds fault and copy the
> instruction in from the instruction pointer, we have absolutely no idea
> what was actually being executed, no matter what mode we are running in.
>
> I believe the instruction decoder already happily handles this.
>
> Furthermore, we don't even *USE* the result of the instruction decode in
> the kernel.  We toss it in to the siginfo and hand it out to userspace.

Hmm.  I may have confused myself.

I was thinking of this:

+ if (is_64bit_mm(mm)) {
+       vaddr_space_size = 1ULL << __VIRTUAL_MASK_SHIFT;
+ bd_entry_virt_space = vaddr_space_size / MPX_BD_NR_ENTRIES_64;
+ /*
+ * __VIRTUAL_MASK takes the 64-bit addressing hole
+ * in to accout.  This is a noop on 32-bit.
+ */
+ addr &= __VIRTUAL_MASK;
+ return addr / bd_entry_virt_space;
+ } else {
+       vaddr_space_size = (1ULL << 32);
+ bd_entry_virt_space = vaddr_space_size / MPX_BD_NR_ENTRIES_32;
+ return addr / bd_entry_virt_space;
+ }

Is there a scenario in which the return value ends up being insanely
high?  If so, does it matter?

--Andy

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Dave Hansen]

On 12/12/2014 05:45 PM, Andy Lutomirski wrote:
> I was thinking of this:
> 
> + if (is_64bit_mm(mm)) {
> +       vaddr_space_size = 1ULL << __VIRTUAL_MASK_SHIFT;
> + bd_entry_virt_space = vaddr_space_size / MPX_BD_NR_ENTRIES_64;
> + /*
> + * __VIRTUAL_MASK takes the 64-bit addressing hole
> + * in to accout.  This is a noop on 32-bit.
> + */
> + addr &= __VIRTUAL_MASK;
> + return addr / bd_entry_virt_space;
> + } else {
> +       vaddr_space_size = (1ULL << 32);
> + bd_entry_virt_space = vaddr_space_size / MPX_BD_NR_ENTRIES_32;
> + return addr / bd_entry_virt_space;
> + }
> 
> Is there a scenario in which the return value ends up being insanely
> high?  If so, does it matter?

Yes, it will be insanely high for a 32-bit process.  The kernel could go
looking for the bounds directory entry at some bonkers virtual address
that makes no sense on 32-bit.

But, that bonkers address is still treated as coming from userspace.
The kernel will go and dereference it via a get_user(), fault, notice
the bad address and kill the process.

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Andy Lutomirski]

On 12/12/2014 11:12 AM, Dave Hansen wrote:
> This is 3.20 material.  I'm hoping to get some comments early
> in case folks have some issues with the way it's being done.
>

Would it make sense to disable MPX for 32-bit binaries on 64-bit kernels 
for 3.19?

--Andy

> --
>
> The MPX hardware structures differ in layout in 32 and 64-bit
> mode.  A 32-bit binary running on a 64-bit kernel needs the
> 32-bit structures, so we need code which switches between
> the two modes.
>
> The first patch is just a documentation update.  The next 5
> patches are really just prepwork.  The meat is in the last 3.
>
> 	x86: make is_64bit_mm() widely available
> 	x86: make __VIRTUAL_MASK safe to use on 32 bit
> 	x86, mpx: we do not allocate the bounds directory
> 	x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK
> 	x86, mpx: Add temporary variable to reduce masking
> 	x86, mpx: new directory entry to addr helper
> 	x86, mpx: do 32-bit-only cmpxchg for 32-bit apps
> 	x86, mpx: support 32bit binaries on 64bit kernel
>
>   Documentation/x86/intel_mpx.txt    |   12 +++-
>   arch/x86/include/asm/mmu_context.h |   13 +++++
>   arch/x86/include/asm/mpx.h         |    2
>   arch/x86/include/asm/page_types.h  |    8 +++
>   arch/x86/kernel/uprobes.c          |   10 ---
>   arch/x86/mm/mpx.c                  |   93 ++++++++++++++++++++++++++++++-------
>   6 files changed, 107 insertions(+), 31 deletions(-)
>

Re: [RFC][PATCH 0/8] x86, mpx: Support 32-bit binaries on 64-bit kernels

[Dave Hansen]

On 12/12/2014 12:27 PM, Andy Lutomirski wrote:
> On 12/12/2014 11:12 AM, Dave Hansen wrote:
>> This is 3.20 material.  I'm hoping to get some comments early
>> in case folks have some issues with the way it's being done.
> 
> Would it make sense to disable MPX for 32-bit binaries on 64-bit kernels
> for 3.19?

Yes, we probably should do something.  It was an oversight that we
didn't do this explicitly in the stuff that got merged.