x86, mpx: Instruction decoder fixes and hardening
v4 cover x86, mpx: Instruction decoder fixes and hardening
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(-)
Signature · 1 line
2.1.0
PATCH v4 1/2 x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
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(-)
arch/x86/mm/mpx.c +16 -9
@@ -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:
Signature · 1 line
2.1.0
Re: [PATCH 3.19 v4 1/2] x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
Quoted 21 lines · 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 <[email protected]>--- arch/x86/mm/mpx.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)arch/x86/mm/mpx.c +5
-0@@ -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
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 247
Quoted 10 lines
arch/x86/mm/mpx.c +10
-0+* 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
Quoted 29 lines · Thomas Gleixner
Quoted 26 lines · 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 <[email protected]>--- arch/x86/mm/mpx.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)arch/x86/mm/mpx.c +10
-0@@ -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 )
Quoted 8 lines
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 247
Quoted 5 lines
arch/x86/mm/mpx.c +5
-0+ 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
Quoted 4 lines
Thanks,
tglx
PATCH v4 2/2 x86: Enforce maximum instruction size in the instruction decoder
In reply to Andy Lutomirskix86 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.
--- arch/x86/lib/insn.c | 7 +++++++ 1 file changed, 7 insertions(+)
arch/x86/lib/insn.c +7 -0
@@ -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;
Signature · 1 line
2.1.0
Re: [PATCH 3.19 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder
Quoted 7 lines · 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.
Hmm, is there any problem to just change MAX_INSN_SIZE to 15?
Quoted 15 lines
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 decoderSigned-off-by Andy Lutomirski <[email protected]>--- arch/x86/lib/insn.c | 7 +++++++ 1 file changed, 7 insertions(+)arch/x86/lib/insn.c +0
-0
arch/x86/lib/insn.c line 61
Quoted 10 lines
arch/x86/lib/insn.c +7
-0@@ -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,
Quoted 4 lines
arch/x86/lib/insn.c +0
-0memset (insn,0 ,sizeof (*insn)); insn->kaddr = kaddr; insn->end_kaddr = kaddr + buf_len;
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]
Re: [PATCH 3.19 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder
Quoted 11 lines · Masami Hiramatsu
Quoted 7 lines · 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.Hmm, is there any problem to just change
MAX_INSN_SIZEto 15?
I don't want to do that for 3.19. It's kind of late.
Quoted 22 lines
Quoted 22 lines
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 decoderSigned-off-by Andy Lutomirski <[email protected]>--- arch/x86/lib/insn.c | 7 +++++++ 1 file changed, 7 insertions(+)arch/x86/lib/insn.c +3
-0@@ -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.
Quoted 8 lines
arch/x86/lib/insn.c line 61
Quoted 4 lines
arch/x86/lib/insn.c +4
-0+*/ +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
Quoted 16 lines
Thank you,
arch/x86/lib/insn.c line 64
Quoted 4 lines
arch/x86/lib/insn.c +0
-0memset (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: [email protected]
Re: [PATCH 3.19 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder
Quoted 47 lines · Andy Lutomirski
Quoted 11 lines · Masami Hiramatsu
Quoted 7 lines · 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.Hmm, is there any problem to just change
MAX_INSN_SIZEto 15?I don't want to do that for 3.19. It's kind of late.
Quoted 22 lines
Quoted 22 lines
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 decoderSigned-off-by Andy Lutomirski <[email protected]>--- arch/x86/lib/insn.c | 7 +++++++ 1 file changed, 7 insertions(+)arch/x86/lib/insn.c +3
-0@@ -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. Quoted 8 lines
arch/x86/lib/insn.c line 61
Quoted 4 lines
arch/x86/lib/insn.c +4
-0+*/ +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.
Quoted 1 line
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.
Quoted 2 lines
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,
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]
Re: [PATCH 3.19 v4 2/2] x86: Enforce maximum instruction size in the instruction decoder
Quoted 60 lines · Masami Hiramatsu
Quoted 47 lines · Andy Lutomirski
Quoted 11 lines · Masami Hiramatsu
Quoted 7 lines · 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.Hmm, is there any problem to just change
MAX_INSN_SIZEto 15?I don't want to do that for 3.19. It's kind of late.
Quoted 22 lines
Quoted 22 lines
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 decoderSigned-off-by Andy Lutomirski <[email protected]>--- arch/x86/lib/insn.c | 7 +++++++ 1 file changed, 7 insertions(+)arch/x86/lib/insn.c +3
-0@@ -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. Quoted 8 lines
arch/x86/lib/insn.c line 61
Quoted 4 lines
arch/x86/lib/insn.c +4
-0+*/ +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.Quoted 1 line
I thought this was your suggestion. Did I misunderstand?
Yes, what I meant about "15" was the the "15" in the comment. So
arch/x86/lib/insn.c +6
-0+/* +* 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 ;
Quoted 8 lines
is acceptable.
Quoted 2 lines
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_SIZEto 15.
Since this has waited for quite a while and there's no known urgent problem, I'll just send a combined patch.
--Andy
Quoted 9 lines
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC