Skip to content

Commit

Permalink
x86: Fix insn decoder for longer instruction
Browse files Browse the repository at this point in the history
Fix x86 insn decoder for hardening against invalid length
instructions. This adds length checkings for each byte-read
site and if it exceeds MAX_INSN_SIZE, returns immediately.
This can happen when decoding user-space binary.

Caller can check whether it happened by checking insn.*.got
member is set or not.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: acme@redhat.com
Cc: ming.m.lin@intel.com
Cc: robert.richter@amd.com
Cc: ravitillo@lbl.gov
Cc: yrl.pp-manager.tt@hitachi.com
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20111007133155.10933.58577.stgit@localhost.localdomain
Signed-off-by: Ingo Molnar <mingo@elte.hu>
  • Loading branch information
Masami Hiramatsu authored and Ingo Molnar committed Oct 10, 2011
1 parent 65112dc commit 53a019a
Showing 1 changed file with 43 additions and 5 deletions.
48 changes: 43 additions & 5 deletions arch/x86/lib/insn.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,23 @@
#include <asm/inat.h>
#include <asm/insn.h>

#define get_next(t, insn) \
({t r; r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
((insn)->next_byte + sizeof(t) + n - (insn)->kaddr <= MAX_INSN_SIZE)

#define __get_next(t, insn) \
({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })

#define __peek_nbyte_next(t, insn, n) \
({ t r = *(t*)((insn)->next_byte + n); r; })

#define peek_next(t, insn) \
({t r; r = *(t*)insn->next_byte; r; })
#define get_next(t, insn) \
({ if (unlikely(!validate_next(t, insn, 0))) goto err_out; __get_next(t, insn); })

#define peek_nbyte_next(t, insn, n) \
({t r; r = *(t*)((insn)->next_byte + n); r; })
({ if (unlikely(!validate_next(t, insn, n))) goto err_out; __peek_nbyte_next(t, insn, n); })

#define peek_next(t, insn) peek_nbyte_next(t, insn, 0)

/**
* insn_init() - initialize struct insn
Expand Down Expand Up @@ -158,6 +167,8 @@ void insn_get_prefixes(struct insn *insn)
insn->vex_prefix.got = 1;

prefixes->got = 1;

err_out:
return;
}

Expand Down Expand Up @@ -208,6 +219,9 @@ void insn_get_opcode(struct insn *insn)
insn->attr = 0; /* This instruction is bad */
end:
opcode->got = 1;

err_out:
return;
}

/**
Expand Down Expand Up @@ -241,6 +255,9 @@ void insn_get_modrm(struct insn *insn)
if (insn->x86_64 && inat_is_force64(insn->attr))
insn->opnd_bytes = 8;
modrm->got = 1;

err_out:
return;
}


Expand Down Expand Up @@ -290,6 +307,9 @@ void insn_get_sib(struct insn *insn)
}
}
insn->sib.got = 1;

err_out:
return;
}


Expand Down Expand Up @@ -351,6 +371,9 @@ void insn_get_displacement(struct insn *insn)
}
out:
insn->displacement.got = 1;

err_out:
return;
}

/* Decode moffset16/32/64 */
Expand All @@ -373,6 +396,9 @@ static void __get_moffset(struct insn *insn)
break;
}
insn->moffset1.got = insn->moffset2.got = 1;

err_out:
return;
}

/* Decode imm v32(Iz) */
Expand All @@ -389,6 +415,9 @@ static void __get_immv32(struct insn *insn)
insn->immediate.nbytes = 4;
break;
}

err_out:
return;
}

/* Decode imm v64(Iv/Ov) */
Expand All @@ -411,6 +440,9 @@ static void __get_immv(struct insn *insn)
break;
}
insn->immediate1.got = insn->immediate2.got = 1;

err_out:
return;
}

/* Decode ptr16:16/32(Ap) */
Expand All @@ -432,6 +464,9 @@ static void __get_immptr(struct insn *insn)
insn->immediate2.value = get_next(unsigned short, insn);
insn->immediate2.nbytes = 2;
insn->immediate1.got = insn->immediate2.got = 1;

err_out:
return;
}

/**
Expand Down Expand Up @@ -496,6 +531,9 @@ void insn_get_immediate(struct insn *insn)
}
done:
insn->immediate.got = 1;

err_out:
return;
}

/**
Expand Down

0 comments on commit 53a019a

Please sign in to comment.