[PATCH 3.19 v2 0/3] x86, mpx: Instruction decoder fixes and hardening

[PATCH 3.19 v2 0/3] x86, mpx: Instruction decoder fixes and hardening

[Andy Lutomirski]

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 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 (3):
  x86: Fix off-by-one in the instruction decoder length checks
  x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
  x86: Enforce MAX_INSN_SIZE in the instruction decoder

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

-- 
2.1.0

[PATCH 3.19 v2 1/3] x86: Fix off-by-one in the instruction decoder length checks

[Andy Lutomirski]

If next_byte + sizeof(t) == end_kaddr, then we are trying to read
exactly to the end; this should be allowed.

Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/lib/insn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 2480978b31cc..1313ae6b478b 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -28,7 +28,7 @@
 
 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
-	((insn)->next_byte + sizeof(t) + n < (insn)->end_kaddr)
+	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
 
 #define __get_next(t, insn)	\
 	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
-- 
2.1.0

[PATCH 3.19 v2 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 at very little complexity cost.

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

diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index 67ebf5751222..3c3692149e43 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -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 cat incorrectly return
+	 * -EINVAL instead of -EFAULT under some circumstances.  This
+	 * discrepency 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

[PATCH 3.19 v2 3/3] x86: Enforce MAX_INSN_SIZE in the instruction decoder

[Andy Lutomirski]

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.

Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

Arguably, the limit could be hard-coded as 15 instead of relying on the
macro.

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

diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 1313ae6b478b..c5912d7a4a15 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -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

Re: [PATCH 3.19 v2 1/3] x86: Fix off-by-one in the instruction decoder length checks

[Dave Hansen]

On 01/12/2015 03:04 PM, Andy Lutomirski wrote:
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 2480978b31cc..1313ae6b478b 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -28,7 +28,7 @@
>  
>  /* Verify next sizeof(t) bytes can be on the same instruction */
>  #define validate_next(t, insn, n)	\
> -	((insn)->next_byte + sizeof(t) + n < (insn)->end_kaddr)
> +	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>  
>  #define __get_next(t, insn)	\
>  	({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })

This issue should already be handled by this patch:

http://git.kernel.org/tip/0f363b250b15af0f218bb2876d101fe5cd413f8b

Re: [PATCH 3.19 v2 1/3] x86: Fix off-by-one in the instruction decoder length checks

[Andy Lutomirski]

On Mon, Jan 12, 2015 at 3:13 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 01/12/2015 03:04 PM, Andy Lutomirski wrote:
>> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
>> index 2480978b31cc..1313ae6b478b 100644
>> --- a/arch/x86/lib/insn.c
>> +++ b/arch/x86/lib/insn.c
>> @@ -28,7 +28,7 @@
>>
>>  /* Verify next sizeof(t) bytes can be on the same instruction */
>>  #define validate_next(t, insn, n)    \
>> -     ((insn)->next_byte + sizeof(t) + n < (insn)->end_kaddr)
>> +     ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>>
>>  #define __get_next(t, insn)  \
>>       ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
>
> This issue should already be handled by this patch:
>
> http://git.kernel.org/tip/0f363b250b15af0f218bb2876d101fe5cd413f8b

I wasn't paying much attention to the world for the last couple weeks,
and I didn't notice that it was already fixed.

Ingo, etc: I won't send a v3 just to drop this patch from the series.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[Dave Hansen]

Couple of typos...

On 01/12/2015 03:04 PM, Andy Lutomirski wrote:
> +	/*
> +	 * 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

"...can incorrectly return"

> +	 * -EINVAL instead of -EFAULT under some circumstances.  This
> +	 * discrepency has no effect.
> +	 */

	^^ discrepancy


> +	if (nr_copied < 2)
> +		goto bad_opcode;
> +	if (buf[0] != 0x0f)
> +		goto bad_opcode;
> +	if (buf[1] != 0x1a && buf[1] != 0x1b)
> +		goto bad_opcode;
...
> -	/*
> -	 * 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;

Otherwise, this looks OK to me.  Have you tested this at all?  I know
you don't have any MPX hardware, but you can still hack something in to
point the instruction decoder at an MPX binary.

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

[Andy Lutomirski]

On Mon, Jan 12, 2015 at 3:47 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> Couple of typos...
>
> On 01/12/2015 03:04 PM, Andy Lutomirski wrote:
>> +     /*
>> +      * 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
>
> "...can incorrectly return"

Will fix.

>
>> +      * -EINVAL instead of -EFAULT under some circumstances.  This
>> +      * discrepency has no effect.
>> +      */
>
>         ^^ discrepancy
>

Will fix.

>
>> +     if (nr_copied < 2)
>> +             goto bad_opcode;
>> +     if (buf[0] != 0x0f)
>> +             goto bad_opcode;
>> +     if (buf[1] != 0x1a && buf[1] != 0x1b)
>> +             goto bad_opcode;
> ...
>> -     /*
>> -      * 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;
>
> Otherwise, this looks OK to me.  Have you tested this at all?  I know
> you don't have any MPX hardware, but you can still hack something in to
> point the instruction decoder at an MPX binary.

I haven't tested this at all.  ISTM it's more likely that any test
hack I write for this will mask any problem than that it will be a
real test.

That being said, it should be okay, given that the condition was
already there later in the function.

--Andy

Re: [PATCH 3.19 v2 3/3] x86: Enforce MAX_INSN_SIZE in the instruction decoder

[Masami Hiramatsu]

(2015/01/13 8:04), Andy Lutomirski wrote:
> 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.

Hm, this patch logic is OK, but the comment is a bit odd.
As you said, if the current code incorrectly set MAX_INSN_SIZE to 16,
the comment should not say "15" without changing MAX_INSN_SIZE itself.

Others are OK for me.

> 
> Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
> 
> Arguably, the limit could be hard-coded as 15 instead of relying on the
> macro.
> 
> arch/x86/lib/insn.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 1313ae6b478b..c5912d7a4a15 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -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

I meant you'd better use MAX_INSN_SIZE instead of "15 bytes" here.

Thank you,

> +	 * 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;
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

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

[Dave Hansen]

On 01/12/2015 03:57 PM, Andy Lutomirski wrote:
>>> >> -     /*
>>> >> -      * 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;
>> >
>> > Otherwise, this looks OK to me.  Have you tested this at all?  I know
>> > you don't have any MPX hardware, but you can still hack something in to
>> > point the instruction decoder at an MPX binary.
> I haven't tested this at all.  ISTM it's more likely that any test
> hack I write for this will mask any problem than that it will be a
> real test.

This is completely and totally broken when there is an instruction
prefix.  Instruction prefixes which occur before the opcodes in the
buffer, so buf[0] is not necessarily insn->opcode.bytes[0].

This was immediately obvious when I actually ran this code for the first
time, even on hardware without MPX.

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

[Andy Lutomirski]

On Wed, Jan 14, 2015 at 11:43 AM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 01/12/2015 03:57 PM, Andy Lutomirski wrote:
>>>> >> -     /*
>>>> >> -      * 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;
>>> >
>>> > Otherwise, this looks OK to me.  Have you tested this at all?  I know
>>> > you don't have any MPX hardware, but you can still hack something in to
>>> > point the instruction decoder at an MPX binary.
>> I haven't tested this at all.  ISTM it's more likely that any test
>> hack I write for this will mask any problem than that it will be a
>> real test.
>
> This is completely and totally broken when there is an instruction
> prefix.  Instruction prefixes which occur before the opcodes in the
> buffer, so buf[0] is not necessarily insn->opcode.bytes[0].
>
> This was immediately obvious when I actually ran this code for the first
> time, even on hardware without MPX.

My apologies -- I misunderstood what insn->opcode.bytes did, and I
assumed that the original code intentionally did the same thing.

Please drop this patch.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC