x86, mpx: Instruction decoder fixes and hardening
v2 cover 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 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(-)
Signature · 1 line
2.1.0
PATCH v2 1/3 x86: Fix off-by-one in the instruction decoder length checks
If next_byte + sizeof(t) == end_kaddr, then we are trying to read exactly to the end; this should be allowed.
--- arch/x86/lib/insn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/lib/insn.c +1 -1
@@ -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; })
Signature · 1 line
2.1.0
Dave Hansen
maintainerRe: [PATCH 3.19 v2 1/3] x86: Fix off-by-one in the instruction decoder length checks
arch/x86/lib/insn.c line 34
Quoted 13 lines · Andy Lutomirski
arch/x86/lib/insn.c +1
-1@@ -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
Quoted 12 lines · Dave Hansen
Quoted 11 lines · Andy Lutomirski
arch/x86/lib/insn.c +1
-1@@ -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)
Quoted 6 lines
arch/x86/lib/insn.c line 34
Quoted 2 lines
arch/x86/lib/insn.c +0
-0#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
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
PATCH v2 2/3 x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
In reply to Andy LutomirskiThis 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 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:
Signature · 1 line
2.1.0
Dave Hansen
maintainerRe: [PATCH 3.19 v2 2/3] x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
Couple of typos...
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 238
Quoted 5 lines · Andy Lutomirski
arch/x86/mm/mpx.c +5
-0+/* +* 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"
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 241
Quoted 3 lines
arch/x86/mm/mpx.c +3
-0+* -EINVAL instead of -EFAULT under some circumstances. This +* discrepency has no effect. +*/
^^ discrepancy
Quoted 6 lines
arch/x86/mm/mpx.c +6
-0+if (nr_copied <2 ) + goto bad_opcode; +if (buf[0 ] !=0x0f ) + goto bad_opcode; +if (buf[1 ] !=0x1a && buf[1 ] !=0x1b ) + goto bad_opcode;
...
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 255
Quoted 9 lines
arch/x86/mm/mpx.c +0
-9-/* -* 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
Quoted 10 lines · Dave Hansen
Couple of typos...
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 238
Quoted 5 lines · Andy Lutomirski
arch/x86/mm/mpx.c +5
-0+/* +* 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.
Quoted 7 lines
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 241
Quoted 4 lines
arch/x86/mm/mpx.c +3
-0+* -EINVAL instead of -EFAULT under some circumstances. This +* discrepency has no effect. +*/ ^^ discrepancy
Will fix.
Quoted 18 lines
Quoted 7 lines
arch/x86/mm/mpx.c +6
-0+if (nr_copied <2 ) + goto bad_opcode; +if (buf[0 ] !=0x0f ) + goto bad_opcode; +if (buf[1 ] !=0x1a && buf[1 ] !=0x1b ) + goto bad_opcode;...
arch/x86/mm/mpx.c static int mpx_insn_decode(struct insn *insn, · line 255
Quoted 9 lines
arch/x86/mm/mpx.c +0
-9-/* -* 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;
Quoted 3 lines
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
Dave Hansen
maintainerRe: [PATCH 3.19 v2 2/3] x86, mpx: Short-circuit the instruction decoder for unexpected opcodes
Quoted 10 lines · Andy Lutomirski
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 9 lines
arch/x86/mm/mpx.c +0
-9-/* -* 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;
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
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
Quoted 18 lines · Dave Hansen
Quoted 10 lines · Andy Lutomirski
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 9 lines
arch/x86/mm/mpx.c +0
-9-/* -* 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;Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 6 lines
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
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
PATCH v2 3/3 x86: Enforce MAX_INSN_SIZE in the instruction decoder
In reply to Andy LutomirskiThe 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.
---
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(+)
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 >MAX_INSN_SIZE ) + buf_len =MAX_INSN_SIZE ; +memset (insn,0 ,sizeof (*insn)); insn->kaddr = kaddr; insn->end_kaddr = kaddr + buf_len;
Signature · 1 line
2.1.0
Re: [PATCH 3.19 v2 3/3] x86: Enforce MAX_INSN_SIZE in the instruction decoder
Quoted 7 lines · Andy Lutomirski
The instruction decoder used to assume that the input buffer was exactly
MAX_INSN_SIZEbytes long. Now that the input buffer has variable length, even if the input buffer is longer thanMAX_INSN_SIZE, we should still reject instructions longer thanMAX_INSN_SIZE, since a real CPU will reject them even if they're otherwise valid.
Quoted 6 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.
It's worth noting that
MAX_INSN_SIZEis 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.
Quoted 16 lines
Fixes 6ba48ff46f76 x86: Remove arbitrary instruction size limit in instruction decoderSigned-off-by Andy Lutomirski <[email protected]>---
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(+)arch/x86/lib/insn.c +0
-0
arch/x86/lib/insn.c line 56
Quoted 5 lines
arch/x86/lib/insn.c +2
-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
I meant you'd better use MAX_INSN_SIZE instead of "15 bytes" here.
Thank you,
Quoted 9 lines
arch/x86/lib/insn.c +5
-0+* 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;
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]