[PATCH 3.19 0/3] Possible MPX improvements for 3.19

[PATCH 3.19 0/3] Possible MPX improvements for 3.19

[Andy Lutomirski]

I don't have the hardware, so this is only compile-tested.

Patches 1 and 2 should be safe.  Patch 1 is a bugfix, although given the
bitness sensitivity of the MPX data structures, any user code that hits
the bug is probably doomed to failure anyway.  Patch 2 is a hardening
change that adds almost no complexity (it's mostly just reordering some
code) that will make it considerably harder to exploit a possibly insn
decoder vulnerability using threads that modify MPX instructions that
send #BR before the trap handler runs.

Patch 3 will be much more controversial, since it's a complete ABI
break.  The ABI in question has never appeared in a released kernel,
though.  If patch 3 is not okay, then I want to fix the prctl
implementation to at least validate its arguments.

Andy Lutomirski (3):
  x86, mpx: Check user mode bitness correctly when decoding instructions
  x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
  x86, mpx: Change the MPX enable/disable API to arch_prctl

 arch/x86/include/asm/ptrace.h     |  5 +++++
 arch/x86/include/uapi/asm/prctl.h | 15 +++++++++++----
 arch/x86/kernel/process_64.c      | 10 ++++++++++
 arch/x86/mm/mpx.c                 | 28 ++++++++++++++++++----------
 include/uapi/linux/prctl.h        |  6 ------
 kernel/sys.c                      | 12 ------------
 6 files changed, 44 insertions(+), 32 deletions(-)

-- 
2.1.0

[PATCH 3.19 1/3] x86, mpx: Check user mode bitness correctly when decoding instructions

[Andy Lutomirski]

When decoding a user instruction, the bitness depends on CS, not on
TIF_IA32.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/ptrace.h | 5 +++++
 arch/x86/mm/mpx.c             | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 86fc2bb82287..189113c74726 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -144,6 +144,11 @@ static inline bool user_64bit_mode(struct pt_regs *regs)
 	(test_thread_flag(TIF_IA32) 	\
 	 ? current_pt_regs()->sp 	\
 	 : this_cpu_read(old_rsp))
+#else
+static inline bool user_64bit_mode(struct pt_regs *regs)
+{
+	return false;
+}
 #endif
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index 67ebf5751222..082ab9c4ac1c 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -217,7 +217,7 @@ static int mpx_insn_decode(struct insn *insn,
 			   struct pt_regs *regs)
 {
 	unsigned char buf[MAX_INSN_SIZE];
-	int x86_64 = !test_thread_flag(TIF_IA32);
+	int x86_64 = user_64bit_mode(regs);
 	int not_copied;
 	int nr_copied;
 
-- 
2.1.0

[PATCH 3.19 2/3] x86, mpx: Short-circuit the instruction decoder for unexpected opcodes

[Andy Lutomirski]

This reduces the degree to which we're exposing the instruction decoder
to malicious user code and very little complexity cost.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/mm/mpx.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index 082ab9c4ac1c..cefa615becb3 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -230,6 +230,23 @@ static int mpx_insn_decode(struct insn *insn,
 	 */
 	if (!nr_copied)
 		return -EFAULT;
+
+	/*
+	 * We only _really_ need to decode bndcl/bndcn/bndcu
+	 * Error out on anything else.  Check this before decoding the
+	 * instruction to reduce our exposure to intentionally bad code
+	 * to some extent.  Note that this shortcut cat incorrectly return
+	 * -EINVAL instead of -EFAULT under some circumstances.  This
+	 * discrepency has no effect.
+	 */
+	if (nr_copied < 2)
+		goto bad_opcode;
+	if (insn->opcode.bytes[0] != 0x0f)
+		goto bad_opcode;
+	if ((insn->opcode.bytes[1] != 0x1a) &&
+	    (insn->opcode.bytes[1] != 0x1b))
+		goto bad_opcode;
+
 	insn_init(insn, buf, nr_copied, x86_64);
 	insn_get_length(insn);
 	/*
@@ -244,15 +261,6 @@ static int mpx_insn_decode(struct insn *insn,
 		return -EFAULT;
 
 	insn_get_opcode(insn);
-	/*
-	 * We only _really_ need to decode bndcl/bndcn/bndcu
-	 * Error out on anything else.
-	 */
-	if (insn->opcode.bytes[0] != 0x0f)
-		goto bad_opcode;
-	if ((insn->opcode.bytes[1] != 0x1a) &&
-	    (insn->opcode.bytes[1] != 0x1b))
-		goto bad_opcode;
 
 	return 0;
 bad_opcode:
-- 
2.1.0

[PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

Enabling and disabling kernel assistance for MPX is as arch-specific
as it gets.  Update the API to use arch_prctl.

This has the benefit the it avoids cluttering prctl with more
arch-specific functionality.  The down side is that arch_prctl will
need to be wired up as a 32-bit syscall to add 32-bit support for
MPX.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/uapi/asm/prctl.h | 15 +++++++++++----
 arch/x86/kernel/process_64.c      | 10 ++++++++++
 include/uapi/linux/prctl.h        |  6 ------
 kernel/sys.c                      | 12 ------------
 4 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
index 3ac5032fae09..a39aef96a922 100644
--- a/arch/x86/include/uapi/asm/prctl.h
+++ b/arch/x86/include/uapi/asm/prctl.h
@@ -1,9 +1,16 @@
 #ifndef _ASM_X86_PRCTL_H
 #define _ASM_X86_PRCTL_H
 
-#define ARCH_SET_GS 0x1001
-#define ARCH_SET_FS 0x1002
-#define ARCH_GET_FS 0x1003
-#define ARCH_GET_GS 0x1004
+#define ARCH_SET_GS		0x1001
+#define ARCH_SET_FS		0x1002
+#define ARCH_GET_FS		0x1003
+#define ARCH_GET_GS		0x1004
+
+/*
+ * Tell the kernel to start/stop helping userspace manage bounds tables.
+ * For both of these functions, addr must be zero.
+ */
+#define ARCH_ENABLE_MPX		0x1005
+#define ARCH_DISABLE_MPX	0x1006
 
 #endif /* _ASM_X86_PRCTL_H */
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 5a2c02913af3..d11355668e58 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -586,6 +586,16 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 		ret = put_user(base, (unsigned long __user *)addr);
 		break;
 	}
+	case ARCH_ENABLE_MPX: {
+		if (addr != 0)
+			return -EINVAL;
+		ret = mpx_enable_management(task);
+	}
+	case ARCH_DISABLE_MPX: {
+		if (addr != 0)
+			return -EINVAL;
+		ret = mpx_disable_management(task);
+	}
 
 	default:
 		ret = -EINVAL;
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 89f63503f903..513df75d0fc9 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -179,10 +179,4 @@ struct prctl_mm_map {
 #define PR_SET_THP_DISABLE	41
 #define PR_GET_THP_DISABLE	42
 
-/*
- * Tell the kernel to start/stop helping userspace manage bounds tables.
- */
-#define PR_MPX_ENABLE_MANAGEMENT  43
-#define PR_MPX_DISABLE_MANAGEMENT 44
-
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index a8c9f5a7dda6..1eaa2f0b0246 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -91,12 +91,6 @@
 #ifndef SET_TSC_CTL
 # define SET_TSC_CTL(a)		(-EINVAL)
 #endif
-#ifndef MPX_ENABLE_MANAGEMENT
-# define MPX_ENABLE_MANAGEMENT(a)	(-EINVAL)
-#endif
-#ifndef MPX_DISABLE_MANAGEMENT
-# define MPX_DISABLE_MANAGEMENT(a)	(-EINVAL)
-#endif
 
 /*
  * this is where the system-wide overflow UID and GID are defined, for
@@ -2209,12 +2203,6 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			me->mm->def_flags &= ~VM_NOHUGEPAGE;
 		up_write(&me->mm->mmap_sem);
 		break;
-	case PR_MPX_ENABLE_MANAGEMENT:
-		error = MPX_ENABLE_MANAGEMENT(me);
-		break;
-	case PR_MPX_DISABLE_MANAGEMENT:
-		error = MPX_DISABLE_MANAGEMENT(me);
-		break;
 	default:
 		error = -EINVAL;
 		break;
-- 
2.1.0

Re: [PATCH 3.19 2/3] x86, mpx: Short-circuit the instruction decoder for unexpected opcodes

[Andy Lutomirski]

On Mon, Dec 29, 2014 at 4:52 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> This reduces the degree to which we're exposing the instruction decoder
> to malicious user code and very little complexity cost.

Don't apply this as is -- it's obviously incorrect.  I'll send a v2 in
a couple of days.

However: can an MPX instruction use a segment override?  FS and GS
seem plausible for TLS.  If so, is the current decoding logic handling
it correctly?  Presumably the byte 0 == 0x0f check will completely
prevent it from working, even if the decode logic would later forget
to correct for the segment base.

--Andy

>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/mm/mpx.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> index 082ab9c4ac1c..cefa615becb3 100644
> --- a/arch/x86/mm/mpx.c
> +++ b/arch/x86/mm/mpx.c
> @@ -230,6 +230,23 @@ static int mpx_insn_decode(struct insn *insn,
>          */
>         if (!nr_copied)
>                 return -EFAULT;
> +
> +       /*
> +        * We only _really_ need to decode bndcl/bndcn/bndcu
> +        * Error out on anything else.  Check this before decoding the
> +        * instruction to reduce our exposure to intentionally bad code
> +        * to some extent.  Note that this shortcut cat incorrectly return
> +        * -EINVAL instead of -EFAULT under some circumstances.  This
> +        * discrepency has no effect.
> +        */
> +       if (nr_copied < 2)
> +               goto bad_opcode;
> +       if (insn->opcode.bytes[0] != 0x0f)
> +               goto bad_opcode;
> +       if ((insn->opcode.bytes[1] != 0x1a) &&
> +           (insn->opcode.bytes[1] != 0x1b))
> +               goto bad_opcode;
> +
>         insn_init(insn, buf, nr_copied, x86_64);
>         insn_get_length(insn);
>         /*
> @@ -244,15 +261,6 @@ static int mpx_insn_decode(struct insn *insn,
>                 return -EFAULT;
>
>         insn_get_opcode(insn);
> -       /*
> -        * We only _really_ need to decode bndcl/bndcn/bndcu
> -        * Error out on anything else.
> -        */
> -       if (insn->opcode.bytes[0] != 0x0f)
> -               goto bad_opcode;
> -       if ((insn->opcode.bytes[1] != 0x1a) &&
> -           (insn->opcode.bytes[1] != 0x1b))
> -               goto bad_opcode;
>
>         return 0;
>  bad_opcode:
> --
> 2.1.0
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Dave Hansen]

On 12/29/2014 04:52 PM, Andy Lutomirski wrote:
> This has the benefit the it avoids cluttering prctl with more
> arch-specific functionality.  The down side is that arch_prctl will
> need to be wired up as a 32-bit syscall to add 32-bit support for
> MPX.

There is existing userspace out there which depends on the existing
prctl() setup.  There isn't a _lot_ and it might still be able to be
changed easily, but this isn't a given.

I'll check in with the folks doing the gcc (runtime) part of this next
week and see what they think.

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andi Kleen]

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

> On 12/29/2014 04:52 PM, Andy Lutomirski wrote:
>> This has the benefit the it avoids cluttering prctl with more
>> arch-specific functionality.  The down side is that arch_prctl will
>> need to be wired up as a 32-bit syscall to add 32-bit support for
>> MPX.
>
> There is existing userspace out there which depends on the existing
> prctl() setup.  There isn't a _lot_ and it might still be able to be
> changed easily, but this isn't a given.
>
> I'll check in with the folks doing the gcc (runtime) part of this next
> week and see what they think.

It'll be quite messy for 32bit because they would need to use syscall(),
as glibc won't have a arch_prctl stub.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Dave Hansen]

On 01/05/2015 12:42 PM, Andi Kleen wrote:
> Dave Hansen <dave.hansen@linux.intel.com> writes:
>> On 12/29/2014 04:52 PM, Andy Lutomirski wrote:
>>> This has the benefit the it avoids cluttering prctl with more
>>> arch-specific functionality.  The down side is that arch_prctl will
>>> need to be wired up as a 32-bit syscall to add 32-bit support for
>>> MPX.
>>
>> There is existing userspace out there which depends on the existing
>> prctl() setup.  There isn't a _lot_ and it might still be able to be
>> changed easily, but this isn't a given.
>>
>> I'll check in with the folks doing the gcc (runtime) part of this next
>> week and see what they think.
> 
> It'll be quite messy for 32bit because they would need to use syscall(),
> as glibc won't have a arch_prctl stub.

Yeah, I'd _really_ prefer not to change it.  The code is in a gcc
branch, but is getting pulled in to the 5.0 release.  We've got
*absolutely* no shortage of prctl numbers.

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Mon, Jan 5, 2015 at 1:18 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
> On 01/05/2015 12:42 PM, Andi Kleen wrote:
>> Dave Hansen <dave.hansen@linux.intel.com> writes:
>>> On 12/29/2014 04:52 PM, Andy Lutomirski wrote:
>>>> This has the benefit the it avoids cluttering prctl with more
>>>> arch-specific functionality.  The down side is that arch_prctl will
>>>> need to be wired up as a 32-bit syscall to add 32-bit support for
>>>> MPX.
>>>
>>> There is existing userspace out there which depends on the existing
>>> prctl() setup.  There isn't a _lot_ and it might still be able to be
>>> changed easily, but this isn't a given.
>>>
>>> I'll check in with the folks doing the gcc (runtime) part of this next
>>> week and see what they think.
>>
>> It'll be quite messy for 32bit because they would need to use syscall(),
>> as glibc won't have a arch_prctl stub.

We should avoid arch_prctl because glibc won't add a syscall stub that
libgcc or whatever would want?  My mind boggles.

>
> Yeah, I'd _really_ prefer not to change it.  The code is in a gcc
> branch, but is getting pulled in to the 5.0 release.  We've got
> *absolutely* no shortage of prctl numbers.

We do, however, have a severe shortage of sanity in the prctl implementation.

Anyway, if it's actually a problem to change it, I have no real
problem keeping it, but I think we *really* need to validate the rest
of the arguments at the very least.

--Andy

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Dave Hansen]

On 01/05/2015 03:04 PM, Andy Lutomirski wrote:
>> > Yeah, I'd _really_ prefer not to change it.  The code is in a gcc
>> > branch, but is getting pulled in to the 5.0 release.  We've got
>> > *absolutely* no shortage of prctl numbers.
> We do, however, have a severe shortage of sanity in the prctl implementation.
> 
> Anyway, if it's actually a problem to change it, I have no real
> problem keeping it, but I think we *really* need to validate the rest
> of the arguments at the very least.

Do you mean just adding a pair of

	if (arg2 || arg3 || arg4 || arg5)
		return -EINVAL;

checks?

That's perfectly fine with me.  I'm happy to queue up a patch to do just
that if that's what you're going for.

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Mon, Jan 5, 2015 at 3:10 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
> On 01/05/2015 03:04 PM, Andy Lutomirski wrote:
>>> > Yeah, I'd _really_ prefer not to change it.  The code is in a gcc
>>> > branch, but is getting pulled in to the 5.0 release.  We've got
>>> > *absolutely* no shortage of prctl numbers.
>> We do, however, have a severe shortage of sanity in the prctl implementation.
>>
>> Anyway, if it's actually a problem to change it, I have no real
>> problem keeping it, but I think we *really* need to validate the rest
>> of the arguments at the very least.
>
> Do you mean just adding a pair of
>
>         if (arg2 || arg3 || arg4 || arg5)
>                 return -EINVAL;
>
> checks?

Exactly.

Thanks,
Andy

>
> That's perfectly fine with me.  I'm happy to queue up a patch to do just
> that if that's what you're going for.



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andi Kleen]

> We should avoid arch_prctl because glibc won't add a syscall stub that
> libgcc or whatever would want?  My mind boggles.

Please turn brain on before posting.

Of course they would add it. But only in the next version. Which means
everyone using older glibc would be out of luck. So all the users
would be stuck using syscall(). Anything you may gain in the kernel
would be totally made up by that.

BTW I added arch_prctl, but in hindsight it wasn't good idea.

-Andi

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Mon, Jan 5, 2015 at 8:04 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> We should avoid arch_prctl because glibc won't add a syscall stub that
>> libgcc or whatever would want?  My mind boggles.
>
> Please turn brain on before posting.

Brain was on.

>
> Of course they would add it. But only in the next version. Which means
> everyone using older glibc would be out of luck. So all the users
> would be stuck using syscall(). Anything you may gain in the kernel
> would be totally made up by that.
>

ISTM libmpx shouldn't link against glibc at all -- what if libmpx
users want to use an alternate runtime (musl, Go, etc.)?

But I decided to check whether libmpx links against glibc, and I can't
find sources for it at all.  Do they exist?  Is there any code with
source available that invokes this prctl?

If not, I personally have very little sympathy for the argument that a
binary buried in the depths of the Intel SDE would need to change if
we switched to using arch_prctl.  And I think that it should issue the
syscall itself without using glibc, in which case the syscall wrapper
issue is moot.

--Andy

> BTW I added arch_prctl, but in hindsight it wasn't good idea.
>
> -Andi



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Dave Hansen]

On 01/05/2015 09:59 PM, Andy Lutomirski wrote:
> But I decided to check whether libmpx links against glibc, and I can't
> find sources for it at all.  Do they exist?  Is there any code with
> source available that invokes this prctl?
> 
> If not, I personally have very little sympathy for the argument that a
> binary buried in the depths of the Intel SDE would need to change if
> we switched to using arch_prctl.  And I think that it should issue the
> syscall itself without using glibc, in which case the syscall wrapper
> issue is moot.

Andy, as I mentioned previously, there is code in a GCC branch that uses
the existing prctl().  It's also been posted for review to one of the
GCC mailing lists.  I've been told that it will be in gcc 5.0.

The Intel SDE does not use the prctl() in any way that I know of.

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Tue, Jan 6, 2015 at 9:48 AM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
> On 01/05/2015 09:59 PM, Andy Lutomirski wrote:
>> But I decided to check whether libmpx links against glibc, and I can't
>> find sources for it at all.  Do they exist?  Is there any code with
>> source available that invokes this prctl?
>>
>> If not, I personally have very little sympathy for the argument that a
>> binary buried in the depths of the Intel SDE would need to change if
>> we switched to using arch_prctl.  And I think that it should issue the
>> syscall itself without using glibc, in which case the syscall wrapper
>> issue is moot.
>
> Andy, as I mentioned previously, there is code in a GCC branch that uses
> the existing prctl().  It's also been posted for review to one of the
> GCC mailing lists.  I've been told that it will be in gcc 5.0.
>

Can you point me to it?  I found the code generation stuff in the gcc
branch, but I couldn't find the runtime.

> The Intel SDE does not use the prctl() in any way that I know of.

I found a couple references suggesting that libmpx lived in the SDE.
For example:

https://software.intel.com/en-us/articles/using-intel-mpx-with-the-intel-software-development-emulator

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Dave Hansen]

On 01/06/2015 10:06 AM, Andy Lutomirski wrote:
> On Tue, Jan 6, 2015 at 9:48 AM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>> On 01/05/2015 09:59 PM, Andy Lutomirski wrote:
>>> But I decided to check whether libmpx links against glibc, and I can't
>>> find sources for it at all.  Do they exist?  Is there any code with
>>> source available that invokes this prctl?
>>>
>>> If not, I personally have very little sympathy for the argument that a
>>> binary buried in the depths of the Intel SDE would need to change if
>>> we switched to using arch_prctl.  And I think that it should issue the
>>> syscall itself without using glibc, in which case the syscall wrapper
>>> issue is moot.
>>
>> Andy, as I mentioned previously, there is code in a GCC branch that uses
>> the existing prctl().  It's also been posted for review to one of the
>> GCC mailing lists.  I've been told that it will be in gcc 5.0.
> 
> Can you point me to it?  I found the code generation stuff in the gcc
> branch, but I couldn't find the runtime.

cc'ing Ilya who is working on the gcc parts...

Ilya, is the MPX runtime that uses the prctl() calls available somewhere
publicly that it can be grabbed?

I couldn't find it in GCC SVN anywhere.

>> The Intel SDE does not use the prctl() in any way that I know of.
> 
> I found a couple references suggesting that libmpx lived in the SDE.
> For example:
> 
> https://software.intel.com/en-us/articles/using-intel-mpx-with-the-intel-software-development-emulator

Andy, I think you're mistaken.  The SDE allows you to run MPX code on a
system without MPX support in hardware.  It does not, itself, provide
MPX code.  The references you see there are to a runtime library that
you obtain separately from the SDE.  You run the library _under_ the SDE.

I hope this clears things up.

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Tue, Jan 6, 2015 at 10:30 AM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 01/06/2015 10:06 AM, Andy Lutomirski wrote:
>> On Tue, Jan 6, 2015 at 9:48 AM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>> On 01/05/2015 09:59 PM, Andy Lutomirski wrote:
>>>> But I decided to check whether libmpx links against glibc, and I can't
>>>> find sources for it at all.  Do they exist?  Is there any code with
>>>> source available that invokes this prctl?
>>>>
>>>> If not, I personally have very little sympathy for the argument that a
>>>> binary buried in the depths of the Intel SDE would need to change if
>>>> we switched to using arch_prctl.  And I think that it should issue the
>>>> syscall itself without using glibc, in which case the syscall wrapper
>>>> issue is moot.
>>>
>>> Andy, as I mentioned previously, there is code in a GCC branch that uses
>>> the existing prctl().  It's also been posted for review to one of the
>>> GCC mailing lists.  I've been told that it will be in gcc 5.0.
>>
>> Can you point me to it?  I found the code generation stuff in the gcc
>> branch, but I couldn't find the runtime.
>
> cc'ing Ilya who is working on the gcc parts...
>
> Ilya, is the MPX runtime that uses the prctl() calls available somewhere
> publicly that it can be grabbed?
>
> I couldn't find it in GCC SVN anywhere.
>
>>> The Intel SDE does not use the prctl() in any way that I know of.
>>
>> I found a couple references suggesting that libmpx lived in the SDE.
>> For example:
>>
>> https://software.intel.com/en-us/articles/using-intel-mpx-with-the-intel-software-development-emulator
>
> Andy, I think you're mistaken.  The SDE allows you to run MPX code on a
> system without MPX support in hardware.  It does not, itself, provide
> MPX code.  The references you see there are to a runtime library that
> you obtain separately from the SDE.  You run the library _under_ the SDE.
>

I misread this:

There are two shared objects in the runtime kit available on the SDE page.

The runtime seems to be here:

https://software.intel.com/protected-download/267266/144917

but I'm not going to accept the EULA.

--Andy

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Dave Hansen]

On 01/06/2015 10:41 AM, Andy Lutomirski wrote:
> The runtime seems to be here:
> 
> https://software.intel.com/protected-download/267266/144917
> 
> but I'm not going to accept the EULA.

That is not the code I'm talking about here.

The SDE doesn't use the prctl()s.  The runtime you're pointing to here
does not use the prctls() (it predates them).  It's not the same as the
code being pushed in to gcc.

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andi Kleen]

On Tue, Jan 06, 2015 at 10:54:26AM -0800, Dave Hansen wrote:
> On 01/06/2015 10:41 AM, Andy Lutomirski wrote:
> > The runtime seems to be here:
> > 
> > https://software.intel.com/protected-download/267266/144917
> > 
> > but I'm not going to accept the EULA.
> 
> That is not the code I'm talking about here.

The MPX run time patch is here (may not be the latest version)

https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01026.html

-Andi

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Tue, Jan 6, 2015 at 11:16 AM, Andi Kleen <andi@firstfloor.org> wrote:
> On Tue, Jan 06, 2015 at 10:54:26AM -0800, Dave Hansen wrote:
>> On 01/06/2015 10:41 AM, Andy Lutomirski wrote:
>> > The runtime seems to be here:
>> >
>> > https://software.intel.com/protected-download/267266/144917
>> >
>> > but I'm not going to accept the EULA.
>>
>> That is not the code I'm talking about here.
>
> The MPX run time patch is here (may not be the latest version)
>
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01026.html
>

Ah, I see.

Given that it doesn't seen to have been committed yet, I'm not too
worried about compatibility.  And "prctl (43)" doesn't actually seem a
whole lot better than "syscall(SYS_arch_prctl, ARCH_ENABLE_MPX, 0)"
(preprocessor macros ftw!).  But I don't feel that strongly about this
point.

--Andy

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andi Kleen]

> Given that it doesn't seen to have been committed yet, I'm not too
> worried about compatibility.  And "prctl (43)" doesn't actually seem a
> whole lot better than "syscall(SYS_arch_prctl, ARCH_ENABLE_MPX, 0)"

This would actually fail with the EINVAL change you requested.

> (preprocessor macros ftw!).  But I don't feel that strongly about this
> point.

-Andi

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Tue, Jan 6, 2015 at 1:22 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> Given that it doesn't seen to have been committed yet, I'm not too
>> worried about compatibility.  And "prctl (43)" doesn't actually seem a
>> whole lot better than "syscall(SYS_arch_prctl, ARCH_ENABLE_MPX, 0)"
>
> This would actually fail with the EINVAL change you requested.
>

So the libmpx code needs to change anyway, then, right?  I really
don't think we should accept garbage in the extra prctl slots just
because uncommitted code somewhere fails to initialize them.

--Andy

>> (preprocessor macros ftw!).  But I don't feel that strongly about this
>> point.
>
> -Andi



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andi Kleen]

On Tue, Jan 06, 2015 at 01:34:56PM -0800, Andy Lutomirski wrote:
> On Tue, Jan 6, 2015 at 1:22 PM, Andi Kleen <andi@firstfloor.org> wrote:
> >> Given that it doesn't seen to have been committed yet, I'm not too
> >> worried about compatibility.  And "prctl (43)" doesn't actually seem a
> >> whole lot better than "syscall(SYS_arch_prctl, ARCH_ENABLE_MPX, 0)"
> >
> > This would actually fail with the EINVAL change you requested.
> >
> 
> So the libmpx code needs to change anyway, then, right?  I really
> don't think we should accept garbage in the extra prctl slots just
> because uncommitted code somewhere fails to initialize them.

Yes it would.

I think that is why most prctls don't do it. After all if you need a new
field you can just add another one. I usually added the checks in the
ones I added, but I can see why not doing it.

-Andi

Re: [PATCH 3.19 1/3] x86, mpx: Check user mode bitness correctly when decoding instructions

[Dave Hansen]

On 12/29/2014 04:52 PM, Andy Lutomirski wrote:
> --- a/arch/x86/mm/mpx.c
> +++ b/arch/x86/mm/mpx.c
> @@ -217,7 +217,7 @@ static int mpx_insn_decode(struct insn *insn,
>  			   struct pt_regs *regs)
>  {
>  	unsigned char buf[MAX_INSN_SIZE];
> -	int x86_64 = !test_thread_flag(TIF_IA32);
> +	int x86_64 = user_64bit_mode(regs);
>  	int not_copied;
>  	int nr_copied;

There are (at least) 3 other uses of the instruction decoder that use
some form of a check on TIF_IA32:

> perf_event_intel_ds.c  intel_pmu_pebs_fixup_ip 785 insn_init(&insn, kaddr, size, is_64bit);
> perf_event_intel_lbr.c branch_type             532 insn_init(&insn, addr, bytes_read, is64);
> uprobes.c              uprobe_init_insn        222 insn_init(insn, auprobe->insn, sizeof(auprobe->insn), x86_64);

Basically doing this:

                is_64bit = kernel_ip(to) || !test_thread_flag(TIF_IA32);

So this method *must* work, at least in practice.

If userspace has MPX on and switches between 32 and 64-bit itself, the
kernel *and* the hardware will suddenly be trying to walk the bounds
tables in the wrong format.  I just don't see an application surviving
very long in that situation.

So I don't have a problem with doing this, long term, as long as we do
it for all of these locations and we do it in a consistent way.  For
MPX, we may even want to enforce that:

	!test_thread_flag(TIF_IA32) == user_64bit_mode(regs)

and freak out if that fails.

But I don't think it's 3.19 material.

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Dave Hansen]

On 01/05/2015 03:04 PM, Andy Lutomirski wrote:
> Anyway, if it's actually a problem to change it, I have no real
> problem keeping it, but I think we *really* need to validate the rest
> of the arguments at the very least.

If we "validate" the arguments like you suggested, then a call like this:

	prctl(PR_MPX_DISABLE_MANAGEMENT);

ends up returning -EINVAL:

> prctl(0x2b /* PR_??? */, 0x7fffffd3, 0x7f360955e9e0, 0x2c, 0x7f3609314840) = -1 EINVAL (Invalid argument)

A quick grep through ltp and some other source I have laying around does
*not* show folks adding 0's to these calls for "empty" arguments.  Is
this really something we want to do?

	prctl(PR_MPX_DISABLE_MANAGEMENT, 0, 0, 0, 0);

Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Andy Lutomirski]

On Wed, Jan 7, 2015 at 3:18 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
> On 01/05/2015 03:04 PM, Andy Lutomirski wrote:
>> Anyway, if it's actually a problem to change it, I have no real
>> problem keeping it, but I think we *really* need to validate the rest
>> of the arguments at the very least.
>
> If we "validate" the arguments like you suggested, then a call like this:
>
>         prctl(PR_MPX_DISABLE_MANAGEMENT);
>
> ends up returning -EINVAL:
>
>> prctl(0x2b /* PR_??? */, 0x7fffffd3, 0x7f360955e9e0, 0x2c, 0x7f3609314840) = -1 EINVAL (Invalid argument)
>
> A quick grep through ltp and some other source I have laying around does
> *not* show folks adding 0's to these calls for "empty" arguments.  Is
> this really something we want to do?
>
>         prctl(PR_MPX_DISABLE_MANAGEMENT, 0, 0, 0, 0);

Yes, because we might want to do prctl(PR_MPX_ENABLE_MANAGEMENT, x, y,
z, 0) some day.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

RE: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

[Enkovich, Ilya]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2666 bytes --]



> -----Original Message-----
> From: Dave Hansen [mailto:dave.hansen@linux.intel.com]
> Sent: Tuesday, January 6, 2015 9:30 PM
> To: Andy Lutomirski
> Cc: Andi Kleen; linux-kernel@vger.kernel.org; X86 ML; Thomas Gleixner;
> Enkovich, Ilya
> Subject: Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API
> to arch_prctl
> 
> On 01/06/2015 10:06 AM, Andy Lutomirski wrote:
> > On Tue, Jan 6, 2015 at 9:48 AM, Dave Hansen
> <dave.hansen@linux.intel.com> wrote:
> >> On 01/05/2015 09:59 PM, Andy Lutomirski wrote:
> >>> But I decided to check whether libmpx links against glibc, and I
> >>> can't find sources for it at all.  Do they exist?  Is there any code
> >>> with source available that invokes this prctl?
> >>>
> >>> If not, I personally have very little sympathy for the argument that
> >>> a binary buried in the depths of the Intel SDE would need to change
> >>> if we switched to using arch_prctl.  And I think that it should
> >>> issue the syscall itself without using glibc, in which case the
> >>> syscall wrapper issue is moot.
> >>
> >> Andy, as I mentioned previously, there is code in a GCC branch that
> >> uses the existing prctl().  It's also been posted for review to one
> >> of the GCC mailing lists.  I've been told that it will be in gcc 5.0.
> >
> > Can you point me to it?  I found the code generation stuff in the gcc
> > branch, but I couldn't find the runtime.
> 
> cc'ing Ilya who is working on the gcc parts...
> 
> Ilya, is the MPX runtime that uses the prctl() calls available somewhere
> publicly that it can be grabbed?
> 
> I couldn't find it in GCC SVN anywhere.

Runtime library is not in GCC trunk yet.  It is available as a patch in GCC mailing list (https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00752.html) and also can be downloaded from Intel Developer Zone (https://software.intel.com/en-us/articles/mpx-support-in-gcc-50).

Ilya

> 
> >> The Intel SDE does not use the prctl() in any way that I know of.
> >
> > I found a couple references suggesting that libmpx lived in the SDE.
> > For example:
> >
> > https://software.intel.com/en-us/articles/using-intel-mpx-with-the-int
> > el-software-development-emulator
> 
> Andy, I think you're mistaken.  The SDE allows you to run MPX code on a
> system without MPX support in hardware.  It does not, itself, provide MPX
> code.  The references you see there are to a runtime library that you obtain
> separately from the SDE.  You run the library _under_ the SDE.
> 
> I hope this clears things up.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 3.19 1/3] x86, mpx: Check user mode bitness correctly when decoding instructions

[Andy Lutomirski]

On Wed, Jan 7, 2015 at 9:22 AM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
> On 12/29/2014 04:52 PM, Andy Lutomirski wrote:
>> --- a/arch/x86/mm/mpx.c
>> +++ b/arch/x86/mm/mpx.c
>> @@ -217,7 +217,7 @@ static int mpx_insn_decode(struct insn *insn,
>>                          struct pt_regs *regs)
>>  {
>>       unsigned char buf[MAX_INSN_SIZE];
>> -     int x86_64 = !test_thread_flag(TIF_IA32);
>> +     int x86_64 = user_64bit_mode(regs);
>>       int not_copied;
>>       int nr_copied;
>
> There are (at least) 3 other uses of the instruction decoder that use
> some form of a check on TIF_IA32:
>
>> perf_event_intel_ds.c  intel_pmu_pebs_fixup_ip 785 insn_init(&insn, kaddr, size, is_64bit);
>> perf_event_intel_lbr.c branch_type             532 insn_init(&insn, addr, bytes_read, is64);
>> uprobes.c              uprobe_init_insn        222 insn_init(insn, auprobe->insn, sizeof(auprobe->insn), x86_64);
>
> Basically doing this:
>
>                 is_64bit = kernel_ip(to) || !test_thread_flag(TIF_IA32);
>
> So this method *must* work, at least in practice.
>
> If userspace has MPX on and switches between 32 and 64-bit itself, the
> kernel *and* the hardware will suddenly be trying to walk the bounds
> tables in the wrong format.  I just don't see an application surviving
> very long in that situation.
>
> So I don't have a problem with doing this, long term, as long as we do
> it for all of these locations and we do it in a consistent way.  For
> MPX, we may even want to enforce that:
>
>         !test_thread_flag(TIF_IA32) == user_64bit_mode(regs)
>
> and freak out if that fails.
>
> But I don't think it's 3.19 material.

Fair enough.  Will send for 3.20 instead.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC