[PATCH 3.19 v4 0/2] x86, mpx: Instruction decoder fixes and hardening

[PATCH 3.19 v4 0/2] x86, mpx: Instruction decoder fixes and hardening

[Andy Lutomirski]

Hi all-

Ingo and Thomas, I think this is in decent shape for x86/urgent now
if you think it's appropriate to apply at this point in the cycle.

Changes from v3:
 - Replaced MAX_INSN_SIZE with 15 in the decoder change.  (Masami)
   I'll send the patch for 3.20 to fix MAX_INSN_SIZE separately.

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 maximum instruction 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

[PATCH 3.19 v4 1/2] 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..a73004330732 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 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

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

[Thomas Gleixner]

On Tue, 13 Jan 2015, Andy Lutomirski wrote:

> 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..a73004330732 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 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;

These opcodes can never have a prefix? If so, then we want to add this
to the comment.

Thanks,

	tglx

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

[Andy Lutomirski]

On Jan 20, 2015 5:59 AM, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>
> On Tue, 13 Jan 2015, Andy Lutomirski wrote:
>
> > 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..a73004330732 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 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;
>
> These opcodes can never have a prefix? If so, then we want to add this
> to the comment.

No, I think I just misunderstood the original code.  Please disregard
this patch.

--Andy

>
> Thanks,
>
>         tglx

[PATCH 3.19 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder

[Andy Lutomirski]

x86 instructions cannot exceed 15 bytes, and the instruction decoder
should enforce that.  Prior to 6ba48ff46f76, the instruction length
limit was implicitly set to 16, which was an approximation of 15,
but there is currently no limit at all.

Fix the decoder to reject instructions that exceed 15 bytes.
A subsequent patch (targetted for 3.20) will fix MAX_INSN_SIZE.

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

Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 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 2480978b31cc..7b80745d2c5a 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 > 15)
+		buf_len = 15;
+
 	memset(insn, 0, sizeof(*insn));
 	insn->kaddr = kaddr;
 	insn->end_kaddr = kaddr + buf_len;
-- 
2.1.0

Re: [PATCH 3.19 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder

[Masami Hiramatsu]

(2015/01/14 6:49), Andy Lutomirski wrote:
> x86 instructions cannot exceed 15 bytes, and the instruction decoder
> should enforce that.  Prior to 6ba48ff46f76, the instruction length
> limit was implicitly set to 16, which was an approximation of 15,
> but there is currently no limit at all.
> 
> Fix the decoder to reject instructions that exceed 15 bytes.
> A subsequent patch (targetted for 3.20) will fix MAX_INSN_SIZE.

Hmm, is there any problem to just change MAX_INSN_SIZE to 15?

> Other than potentially confusing some of the decoder sanity checks,
> I'm not aware of any actual problems that omitting this check would
> cause.
> 
> Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  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 2480978b31cc..7b80745d2c5a 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 > 15)
> +		buf_len = 15;
> +

Without changing the MAX_INSN_SIZE, this looks very odd, since all other
code suppose that the max length of an instruction is 16 (MAX_INSN_SIZE)
except here.

Thank you,

>  	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 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder

[Andy Lutomirski]

On Jan 15, 2015 4:37 AM, "Masami Hiramatsu"
<masami.hiramatsu.pt@hitachi.com> wrote:
>
> (2015/01/14 6:49), Andy Lutomirski wrote:
> > x86 instructions cannot exceed 15 bytes, and the instruction decoder
> > should enforce that.  Prior to 6ba48ff46f76, the instruction length
> > limit was implicitly set to 16, which was an approximation of 15,
> > but there is currently no limit at all.
> >
> > Fix the decoder to reject instructions that exceed 15 bytes.
> > A subsequent patch (targetted for 3.20) will fix MAX_INSN_SIZE.
>
> Hmm, is there any problem to just change MAX_INSN_SIZE to 15?

I don't want to do that for 3.19.  It's kind of late.

>
> > Other than potentially confusing some of the decoder sanity checks,
> > I'm not aware of any actual problems that omitting this check would
> > cause.
> >
> > Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
> > Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> > ---
> >  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 2480978b31cc..7b80745d2c5a 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 > 15)
> > +             buf_len = 15;
> > +
>
> Without changing the MAX_INSN_SIZE, this looks very odd, since all other
> code suppose that the max length of an instruction is 16 (MAX_INSN_SIZE)
> except here.

I thought this was your suggestion.  Did I misunderstand?

If you think the current code is okay for 3.19, I can fold the two
patches together and send for 3.20.

--Andy

>
> Thank you,
>
> >       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 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder

[Masami Hiramatsu]

(2015/01/16 0:22), Andy Lutomirski wrote:
> On Jan 15, 2015 4:37 AM, "Masami Hiramatsu"
> <masami.hiramatsu.pt@hitachi.com> wrote:
>>
>> (2015/01/14 6:49), Andy Lutomirski wrote:
>>> x86 instructions cannot exceed 15 bytes, and the instruction decoder
>>> should enforce that.  Prior to 6ba48ff46f76, the instruction length
>>> limit was implicitly set to 16, which was an approximation of 15,
>>> but there is currently no limit at all.
>>>
>>> Fix the decoder to reject instructions that exceed 15 bytes.
>>> A subsequent patch (targetted for 3.20) will fix MAX_INSN_SIZE.
>>
>> Hmm, is there any problem to just change MAX_INSN_SIZE to 15?
> 
> I don't want to do that for 3.19.  It's kind of late.
> 
>>
>>> Other than potentially confusing some of the decoder sanity checks,
>>> I'm not aware of any actual problems that omitting this check would
>>> cause.
>>>
>>> Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
>>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>>> ---
>>>  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 2480978b31cc..7b80745d2c5a 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 > 15)
>>> +             buf_len = 15;
>>> +
>>
>> Without changing the MAX_INSN_SIZE, this looks very odd, since all other
>> code suppose that the max length of an instruction is 16 (MAX_INSN_SIZE)
>> except here.
> 
> I thought this was your suggestion.  Did I misunderstand?

Yes, what I meant about "15" was the the "15" in the comment.
So

+     /*
+      * Instructions longer than MAX_INSN_SIZE 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;

is acceptable.

> If you think the current code is okay for 3.19, I can fold the two
> patches together and send for 3.20.

If it does really cause a bug or a real problem, it must fix asap.
If not, I'd like to fix this issue with changing MAX_INSN_SIZE to 15.

Thank you,

-- 
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 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder

[Andy Lutomirski]

On Thu, Jan 15, 2015 at 5:02 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> (2015/01/16 0:22), Andy Lutomirski wrote:
>> On Jan 15, 2015 4:37 AM, "Masami Hiramatsu"
>> <masami.hiramatsu.pt@hitachi.com> wrote:
>>>
>>> (2015/01/14 6:49), Andy Lutomirski wrote:
>>>> x86 instructions cannot exceed 15 bytes, and the instruction decoder
>>>> should enforce that.  Prior to 6ba48ff46f76, the instruction length
>>>> limit was implicitly set to 16, which was an approximation of 15,
>>>> but there is currently no limit at all.
>>>>
>>>> Fix the decoder to reject instructions that exceed 15 bytes.
>>>> A subsequent patch (targetted for 3.20) will fix MAX_INSN_SIZE.
>>>
>>> Hmm, is there any problem to just change MAX_INSN_SIZE to 15?
>>
>> I don't want to do that for 3.19.  It's kind of late.
>>
>>>
>>>> Other than potentially confusing some of the decoder sanity checks,
>>>> I'm not aware of any actual problems that omitting this check would
>>>> cause.
>>>>
>>>> Fixes: 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoder
>>>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>>>> ---
>>>>  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 2480978b31cc..7b80745d2c5a 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 > 15)
>>>> +             buf_len = 15;
>>>> +
>>>
>>> Without changing the MAX_INSN_SIZE, this looks very odd, since all other
>>> code suppose that the max length of an instruction is 16 (MAX_INSN_SIZE)
>>> except here.
>>
>> I thought this was your suggestion.  Did I misunderstand?
>
> Yes, what I meant about "15" was the the "15" in the comment.
> So
>
> +     /*
> +      * Instructions longer than MAX_INSN_SIZE 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;
>
> is acceptable.
>
>> If you think the current code is okay for 3.19, I can fold the two
>> patches together and send for 3.20.
>
> If it does really cause a bug or a real problem, it must fix asap.
> If not, I'd like to fix this issue with changing MAX_INSN_SIZE to 15.
>

Since this has waited for quite a while and there's no known urgent
problem, I'll just send a combined patch.

--Andy

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



-- 
Andy Lutomirski
AMA Capital Management, LLC