x86, mpx: Instruction decoder fixes and hardening

Hi all-

Sorry, this is later than I had hoped to send this. This series has three minor fixes for mpx and the instruction decoder.

Changes from v2:
- Dropped patch 1 (fixed in tip separately)
- Fixed comment typoes in patch 2 (noticed by Dave)

Changes from v1:
- Dropped the TIF_IA32 change -- let's defer that until at least 3.20.
- Fixed the MPX decode short-circuit. v1 was buggy.
- Patch 3 is new. It fixes a minor regression from the MPX work.

Andy Lutomirski (2):
x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
x86: Enforce MAX_INSN_SIZE in the instruction decoder

 arch/x86/lib/insn.c |  7 +++++++
 arch/x86/mm/mpx.c   | 25 ++++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)

2.1.0

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

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

index 67ebf5751222..a73004330732 100644
@@ -230,6 +230,22 @@ 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 can incorrectly return
+	 * -EINVAL instead of -EFAULT under some circumstances.  This
+	 * discrepancy has no effect.
+	 */
+	if (nr_copied < 2)
+		goto bad_opcode;
+	if (buf[0] != 0x0f)
+		goto bad_opcode;
+	if (buf[1] != 0x1a && buf[1] != 0x1b)
+		goto bad_opcode;
+
 	insn_init(insn, buf, nr_copied, x86_64);
 	insn_get_length(insn);
 	/*

@@ -244,15 +260,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

The instruction decoder used to assume that the input buffer was exactly MAX_INSN_SIZE bytes long. Now that the input buffer has variable length, even if the input buffer is longer than MAX_INSN_SIZE, we should still reject instructions longer than MAX_INSN_SIZE, since a real CPU will reject them even if they're otherwise valid.

Other than potentially confusing some of the decoder sanity checks, I'm not aware of any actual problems that omitting this check would cause.

It's worth noting that MAX_INSN_SIZE is incorrectly set to 16. This patch doesn't change that. I'll submit a fix for that later.

---
 arch/x86/lib/insn.c | 7 +++++++
 1 file changed, 7 insertions(+)

index 2480978b31cc..4dc8252b6454 100644
@@ -52,6 +52,13 @@
  */
 void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
 {
+	/*
+	 * Instructions longer than 15 bytes are invalid even if the
+	 * input buffer is long enough to hold them.
+	 */
+	if (buf_len > MAX_INSN_SIZE)
+		buf_len = MAX_INSN_SIZE;
+
 	memset(insn, 0, sizeof(*insn));
 	insn->kaddr = kaddr;
 	insn->end_kaddr = kaddr + buf_len;

2.1.0