x86, mpx: Fixes for 32-bit userspace

PATCH New Dave Hansen 2 participants 5 messages Nov 11, 2015 – Nov 12, 2015

Subsystems (3)

Stable branch x86 x86 MM

lore

Share & tools

lore

Dave Hansen

author

2015-11-11 18:19 UTC #0 lore raw

cover x86, mpx: Fixes for 32-bit userspace

These fix up two relatively minor issues that show up when running 32-bit kernels on 64-bit hardware, or 32-bit binaries on 64-bit kernels.

 mpx.c |   47 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

Dave Hansen (2):
x86, mpx: do proper get_user() when running 32-bit binaries on 64-bit x86, mpx: fix 32-bit address space calculation

When we reach out to userspace to try to zap an entire "bounds table" we need to go read a "bounds directory entry" in order to locate the table's address. The size of a "directory entry" depends on the binary being run and is always the size of a pointer.

But, when we have a 64-bit kernel and a 32-bit application, the directory entry is still only 32-bits long, but we fetch it with a 64-bit pointer which makes get_user() does a 64-bit fetch. Reading 4 extra bytes isn't harmful, unless we are at the end of and run off the table. It might also cause the zero page to get faulted in unnecessarily even if you are not at the end.

Fix it up by doing a special 32-bit get_user() via a cast when we have 32-bit userspace.

Signed-off-by Dave Hansen <[email protected]>

---

 b/arch/x86/mm/mpx.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

arch/x86/mm/mpx.c renamed from arch/x86/mm/mpx.c~get_bd_entry +24 -1

diff -puN arch/x86/mm/mpx.c~get_bd_entry arch/x86/mm/mpx.c
@@ -586,6 +586,29 @@ static unsigned long mpx_bd_entry_to_bt_
 }
 
 /*
+ * We only want to do a 4-byte get_user() on 32-bit.  Otherwise,
+ * we might run off the end of the bounds table if we are on
+ * a 64-bit kernel and try to get 8 bytes.
+ */
+int get_user_bd_entry(struct mm_struct *mm, unsigned long *bd_entry_ret,
+		long __user *bd_entry_ptr)
+{
+	u32 bd_entry_32;
+	int ret;
+
+	if (is_64bit_mm(mm))
+		return get_user(*bd_entry_ret, bd_entry_ptr);
+
+	/*
+	 * Note that get_user() uses the type of the *pointer* to
+	 * establish the size of the get, not the destination.
+	 */
+	ret = get_user(bd_entry_32, (u32 __user *)bd_entry_ptr);
+	*bd_entry_ret = bd_entry_32;
+	return ret;
+}
+
+/*
  * Get the base of bounds tables pointed by specific bounds
  * directory entry.
  */

@@ -605,7 +628,7 @@ static int get_bt_addr(struct mm_struct
 		int need_write = 0;
 
 		pagefault_disable();
-		ret = get_user(bd_entry, bd_entry_ptr);
+		ret = get_user_bd_entry(mm, &bd_entry, bd_entry_ptr);
 		pagefault_enable();
 		if (!ret)
 			break;

tip-bot for Dave Hansen

merge bot

2015-11-12 13:27 UTC #2 lore raw

[tip:x86/urgent] x86/mpx: Do proper get_user() when running 32-bit binaries on 64-bit kernels

x86/mpx: Do proper get_user() when running 32-bit binaries on 64-bit kernels

Commit-ID:  46561c3959d6307d22139c24cd0bf196162e5681
Gitweb:     http://git.kernel.org/tip/46561c3959d6307d22139c24cd0bf196162e5681
Author:     Dave Hansen <[email protected]>
AuthorDate: Wed, 11 Nov 2015 10:19:31 -0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 12 Nov 2015 09:20:37 +0100

x86/mpx: Do proper get_user() when running 32-bit binaries on 64-bit kernels

When you call get_user(foo, bar), you effectively do a

	copy_from_user(&foo, bar, sizeof(*bar));

Note that the sizeof() is implicit.

Fix it up by doing a special 32-bit get_user() via a cast when we have 32-bit userspace.

Signed-off-by Dave Hansen <[email protected]>

Cc <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/mm/mpx.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

arch/x86/mm/mpx.c +24 -1

index b0ae85f..6127c5e 100644
@@ -586,6 +586,29 @@ static unsigned long mpx_bd_entry_to_bt_addr(struct mm_struct *mm,
 }
 
 /*
+ * We only want to do a 4-byte get_user() on 32-bit.  Otherwise,
+ * we might run off the end of the bounds table if we are on
+ * a 64-bit kernel and try to get 8 bytes.
+ */
+int get_user_bd_entry(struct mm_struct *mm, unsigned long *bd_entry_ret,
+		long __user *bd_entry_ptr)
+{
+	u32 bd_entry_32;
+	int ret;
+
+	if (is_64bit_mm(mm))
+		return get_user(*bd_entry_ret, bd_entry_ptr);
+
+	/*
+	 * Note that get_user() uses the type of the *pointer* to
+	 * establish the size of the get, not the destination.
+	 */
+	ret = get_user(bd_entry_32, (u32 __user *)bd_entry_ptr);
+	*bd_entry_ret = bd_entry_32;
+	return ret;
+}
+
+/*
  * Get the base of bounds tables pointed by specific bounds
  * directory entry.
  */

@@ -605,7 +628,7 @@ static int get_bt_addr(struct mm_struct *mm,
 		int need_write = 0;
 
 		pagefault_disable();
-		ret = get_user(bd_entry, bd_entry_ptr);
+		ret = get_user_bd_entry(mm, &bd_entry, bd_entry_ptr);
 		pagefault_enable();
 		if (!ret)
 			break;

Dave Hansen

author

2015-11-11 18:19 UTC #3 lore raw

PATCH 2/2 x86, mpx: fix 32-bit address space calculation

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

I received a bug report that running 32-bit MPX binaries on 64-bit kernels was broken. I traced it down to this little code snippet. We were switching our "number of bounds directory entries" calculation correctly. But, we didn't switch the other side of the calculation: the virtual space size.

This meant that we were calculating an absurd size for bd_entry_virt_space() on 32-bit because we used the 64-bit virt_space.

This was _also_ broken for 32-bit kernels running on 64-bit hardware since boot_cpu_data.x86_virt_bits=48 even when running in 32-bit mode.

Correct that and properly handle all 3 possible cases:

1. 32-bit binary on 64-bit kernel
2. 64-bit binary on 64-bit kernel
3. 32-bit binary on 32-bit kernel

This manifested in having bounds tables not properly unmapped. It "leaked" memory but had no functional impact otherwise.

Signed-off-by Dave Hansen <[email protected]>

---

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

arch/x86/mm/mpx.c renamed from arch/x86/mm/mpx.c~x86-mpx-fix-32-bit-address-space-calculation +17 -5

diff -puN arch/x86/mm/mpx.c~x86-mpx-fix-32-bit-address-space-calculation arch/x86/mm/mpx.c
@@ -723,11 +723,23 @@ static unsigned long mpx_get_bt_entry_of
  */
 static inline unsigned long bd_entry_virt_space(struct mm_struct *mm)
 {
-	unsigned long long virt_space = (1ULL << boot_cpu_data.x86_virt_bits);
-	if (is_64bit_mm(mm))
-		return virt_space / MPX_BD_NR_ENTRIES_64;
-	else
-		return virt_space / MPX_BD_NR_ENTRIES_32;
+	unsigned long long virt_space;
+	unsigned long long GB = (1ULL << 30);
+
+	/*
+	 * This covers 32-bit emulation as well as 32-bit kernels
+	 * running on 64-bit harware.
+	 */
+	if (!is_64bit_mm(mm))
+		return (4ULL * GB) / MPX_BD_NR_ENTRIES_32;
+
+	/*
+	 * 'x86_virt_bits' returns what the hardware is capable
+	 * of, and returns the full >32-bit adddress space when
+	 * running 32-bit kernels on 64-bit hardware.
+	 */
+	virt_space = (1ULL << boot_cpu_data.x86_virt_bits);
+	return virt_space / MPX_BD_NR_ENTRIES_64;
 }
 
 /*

tip-bot for Dave Hansen

merge bot

2015-11-12 13:27 UTC #4 lore raw

[tip:x86/urgent] x86/mpx: Fix 32-bit address space calculation

x86/mpx: Fix 32-bit address space calculation

Commit-ID:  f3119b830264d89d216bfb378ab65065dffa02d9
Gitweb:     http://git.kernel.org/tip/f3119b830264d89d216bfb378ab65065dffa02d9
Author:     Dave Hansen <[email protected]>
AuthorDate: Wed, 11 Nov 2015 10:19:34 -0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 12 Nov 2015 09:20:37 +0100

x86/mpx: Fix 32-bit address space calculation

This meant that we were calculating an absurd size for bd_entry_virt_space() on 32-bit because we used the 64-bit virt_space.

This was _also_ broken for 32-bit kernels running on 64-bit hardware since boot_cpu_data.x86_virt_bits=48 even when running in 32-bit mode.

Correct that and properly handle all 3 possible cases:

1. 32-bit binary on 64-bit kernel
2. 64-bit binary on 64-bit kernel
3. 32-bit binary on 32-bit kernel

This manifested in having bounds tables not properly unmapped. It "leaked" memory but had no functional impact otherwise.

Signed-off-by Dave Hansen <[email protected]>

Cc <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/mm/mpx.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

arch/x86/mm/mpx.c +17 -5

index 6127c5e..1202d5c 100644
@@ -723,11 +723,23 @@ static unsigned long mpx_get_bt_entry_offset_bytes(struct mm_struct *mm,
  */
 static inline unsigned long bd_entry_virt_space(struct mm_struct *mm)
 {
-	unsigned long long virt_space = (1ULL << boot_cpu_data.x86_virt_bits);
-	if (is_64bit_mm(mm))
-		return virt_space / MPX_BD_NR_ENTRIES_64;
-	else
-		return virt_space / MPX_BD_NR_ENTRIES_32;
+	unsigned long long virt_space;
+	unsigned long long GB = (1ULL << 30);
+
+	/*
+	 * This covers 32-bit emulation as well as 32-bit kernels
+	 * running on 64-bit harware.
+	 */
+	if (!is_64bit_mm(mm))
+		return (4ULL * GB) / MPX_BD_NR_ENTRIES_32;
+
+	/*
+	 * 'x86_virt_bits' returns what the hardware is capable
+	 * of, and returns the full >32-bit adddress space when
+	 * running 32-bit kernels on 64-bit hardware.
+	 */
+	virt_space = (1ULL << boot_cpu_data.x86_virt_bits);
+	return virt_space / MPX_BD_NR_ENTRIES_64;
 }
 
 /*