Possible MPX improvements for 3.19

arch/x86/mm/mpx.c +0 -0

 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);

arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 222

Quoted 10 lines · Andy Lutomirski

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

@@ -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:

Quoted 3 lines

arch/x86/mm/mpx.c +0 -0

 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.

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 <[email protected]>

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

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

index 082ab9c4ac1c..cefa615becb3 100644
@@ -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

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.

Quoted 4 lines · Andy Lutomirski

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.

Quoted 10 lines · Dave Hansen

Quoted 4 lines · Andy Lutomirski

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.

Quoted 2 lines

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.

Quoted 11 lines · Andi Kleen

Quoted 10 lines · Dave Hansen

Quoted 4 lines · Andy Lutomirski

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 8 lines · Andy Lutomirski

Quoted 3 lines

Quoted 3 lines

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.

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

Quoted 2 lines

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

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.

Quoted 9 lines · Andy Lutomirski

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.

Quoted 11 lines · Dave Hansen

Quoted 9 lines · Andy Lutomirski

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.

Quoted 3 lines

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.

Quoted 1 line

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

Quoted 12 lines · Andy Lutomirski

Quoted 11 lines · Dave Hansen

Quoted 9 lines · Andy Lutomirski

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 1 line

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:

Quoted 1 line

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.

The runtime seems to be here:

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

but I'm not going to accept the EULA.

Quoted 5 lines · Andy Lutomirski

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.

Quoted 5 lines · Dave Hansen

Quoted 4 lines · Andy Lutomirski

The runtime seems to be here:

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

Quoted 3 lines

Quoted 1 line

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

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.

Quoted 3 lines

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.

Quoted 2 lines

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

-Andi

Quoted 6 lines · Andi Kleen

Quoted 3 lines

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.

-----Original Message-----
From: Dave Hansen [mailto:[email protected]]
Sent: Tuesday, January 6, 2015 9:30 PM
To: Andy Lutomirski
Cc: Andi Kleen; [email protected]; X86 ML; Thomas Gleixner; Enkovich, Ilya
Subject: Re: [PATCH 3.19 3/3] x86, mpx: Change the MPX enable/disable API to arch_prctl

Quoted 1 line · Andy Lutomirski

On Tue, Jan 6, 2015 at 9:48 AM, Dave Hansen

Quoted 11 lines · <[email protected]> wrote:

Quoted 11 lines · Andy Lutomirski

Quoted 9 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

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.

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

Quoted 3 lines · Andy Lutomirski

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:

Quoted 1 line

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);