instruction-analysis API(s)
Masami Hiramatsu
mhiramat at redhat.com
Thu Mar 5 23:01:12 UTC 2009
Hi Jim and Sriker,
Here, I almost rewrote my patch.
Changelog:
- rewrite decoding logic based on Intel' manual.
- supoort insn_get_sib(),insn_get_displacement()
and insn_get_immediate() too.
- support 3 bytes opcode and 64bit immediate.
- introduce some bitmaps.
Thank you,
Masami Hiramatsu wrote:
> Hi Jim,
>
> Jim Keniston wrote:
>> On Fri, 2009-02-27 at 16:20 -0500, Masami Hiramatsu wrote:
>> ...
>>> Here are a patch against your code and an example code for
>>> instruction length decoder.
>>> Curiously, KVM's instruction decoder does not completely
>>> cover all instructions(especially, Jcc/test...).
>>> I had to refer Intel manuals.
>>>
>>> Moreover, even with this patch, the decoder is incomplete.
>>> - this doesn't cover 3bytes opcode yet.
>>> - this doesn't decode sib, displacement and immediate.
>>> - might have some bugs :-(
>>>
>>>
>>> Thank you,
>> Thanks for your work on this. Comments below.
>
> Thank you very much for review!
>
> Actually, that code was based on KVM code, so I also found many
> opcodes were not supported.
>
>> As I mentioned in private email, you or I should probably refactor this
>> into:
>> - insn_get_sib()
>> - insn_get_displacement()
>> - insn_get_immediate()
>> - insn_get_length()
>
> Agreed, these should be supported.
>
> I also would like to change struct insn as below;
>
> struct insn {
> struct insn_field prefixes; /* prefixes.value is a bitmap */
> struct insn_field opcode; /* opcode.bytes[n] == opcode_n */
> struct insn_field modrm;
> struct insn_field sib;
> struct insn_field displacement;
> union {
> struct insn_field immediate;
> struct insn_field moffset1; /* for 64bit MOV */
> struct insn_field immediate1; /* for 64bit imm or off16/32 */
> };
> union {
> struct insn_field moffset2; /* for 64bit MOV */
> struct insn_field immediate2; /* for 64bit imm or seg16 */
> };
>
> u8 opnd_bytes;
> u8 addr_bytes;
> u8 length;
> bool x86_64;
>
> const u8 *kaddr; /* kernel address of insn (copy) to analyze */
> const u8 *next_byte;
> };
>
> opcode2 and opcode3 will be stored in opcode.value with opcode1.
>
> Now, I'm updating my code. Would anyone also be working on it?
>
> Thank you,
>
>> Jim
>>
>>> plain text document attachment (insn_x86.patch)
>>> Index: insn_x86.h
>>> ===================================================================
>>> --- insn_x86.h (revision 1510)
>>> +++ insn_x86.h (working copy)
>>> @@ -66,6 +66,10 @@
>>> struct insn_field displacement;
>>> struct insn_field immediate;
>>>
>>> + u8 op_bytes;
>> I'd probably use opnd_bytes and addr_bytes here, for clarity. (When I
>> first saw "op", I thought "opcode".) Also, we should clarify that these
>> are the EFFECTIVE lengths, not the lengths of the immediate and
>> displacement fields in the instruction.
>>
>>> + u8 ad_bytes;
>>> + u8 length;
>>> +
>>> const u8 *kaddr; /* kernel address of insn (copy) to analyze */
>>> const u8 *next_byte;
>>> bool x86_64;
>>> @@ -75,6 +79,7 @@
>>> extern void insn_get_prefixes(struct insn *insn);
>>> extern void insn_get_opcode(struct insn *insn);
>>> extern void insn_get_modrm(struct insn *insn);
>>> +extern void insn_get_length(struct insn *insn);
>>>
>>> #ifdef CONFIG_X86_64
>>> extern bool insn_rip_relative(struct insn *insn);
>>> Index: insn_x86.c
>>> ===================================================================
>>> --- insn_x86.c (revision 1510)
>>> +++ insn_x86.c (working copy)
>>> @@ -17,7 +17,7 @@
>>> *
>>> * Copyright (C) IBM Corporation, 2002, 2004, 2009
>>> */
>>> -
>>> +#include <linux/module.h>
>>> #include <linux/string.h>
>>> // #include <asm/insn.h>
>>> #include "insn_x86.h"
>>> @@ -34,6 +34,11 @@
>>> insn->kaddr = kaddr;
>>> insn->next_byte = kaddr;
>>> insn->x86_64 = x86_64;
>>> + insn->op_bytes = 4;
>>> + if (x86_64)
>>> + insn->ad_bytes = 8;
>>> + else
>>> + insn->ad_bytes = 4;
>>> }
>>> EXPORT_SYMBOL_GPL(insn_init);
>>>
>>> @@ -79,10 +84,51 @@
>>> break;
>>> prefixes->value |= pfx;
>>> }
>>> + if (prefixes->value & X86_PFX_OPNDSZ) {
>>> + /* oprand size switches 2/4 */
>>> + insn->op_bytes ^= 6;
>>> + }
>>> + if (prefixes->value & X86_PFX_ADDRSZ) {
>>> + /* address size switches 2/4 or 4/8 */
>>> +#ifdef CONFIG_X86_64
>>> + if (insn->x86_64)
>>> + insn->op_bytes ^= 12;
>>> + else
>>> +#endif
>>> + insn->op_bytes ^= 6;
>> This seems wrong. You're checking the address-size prefix, but
>> adjusting the operand size.
>>
>>> + }
>>> +#ifdef CONFIG_X86_64
>>> + if (prefixes->value & X86_PFX_REXW)
>>> + insn->op_bytes = 8;
>>> +#endif
>>> prefixes->got = true;
>>> }
>>> EXPORT_SYMBOL_GPL(insn_get_prefixes);
>>>
>>> +static bool __insn_is_stack(struct insn *insn)
>> It's not entirely clear to me what this function checks. (A more
>> precise name might help.) You have pushes, pops, and calls here, but
>> you also have some instructions that don't appear to affect the stack at
>> all. And other push and pop instructions are missing.
>>
>>> +{
>>> + u8 reg;
>>> + if (insn->opcode.nbytes == 2)
>>> + return 0;
>> The following are 2-byte pushes or pops: 0f-a0, 0f-a1, 0f-a8, and 0f-a9.
>>
>> Also, since the return value is bool, I'd prefer to see true/false
>> rather than 1/0.
>>
>>> +
>>> + switch(insn->opcode1) {
>>> + case 0x68:
>>> + case 0x6a:
>>> + case 0x9c:
>>> + case 0x9d:
>>> + case 0xc5:
>> 0xc5 = lds. Why lds?
>>
>> In general, it'd be nice to add a comment showing the mnemonic next to
>> each hex value -- e.g.,
>> case 0x68: /* push */
>>
>>> + case 0xe8:
>>> + return 1;
>>> + }
>> Other related instructions: 9a, 1f, 07, 17, 8f.
>>
>>> + reg = ((*insn->next_byte) >> 3) & 7;
>>> + if ((insn->opcode1 & 0xf0) == 0x50 ||
>>> + (insn->opcode1 == 0x1a && reg == 0) ||
>> The above line doesn't seem right. It catches things like
>> sbb (%rax),%al .
>>
>>> + (insn->opcode1 == 0xff && (reg & 1) == 0 && reg != 0)) {
>> Looks like the interesting reg values are 2 (call), 3 (call), and 6
>> (push).
>>
>>> + return 1;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> /**
>>> * insn_get_opcode - collect opcode(s)
>>> * @insn: &struct insn containing instruction
>>> @@ -108,6 +154,8 @@
>>> opcode->nbytes = 1;
>>> opcode->value = insn->opcode1;
>>> opcode->got = true;
>>> + if (insn->x86_64 && __insn_is_stack(insn))
>>> + insn->op_bytes = 8;
>>> }
>>> EXPORT_SYMBOL_GPL(insn_get_opcode);
>>>
>>> @@ -208,3 +256,115 @@
>>> }
>>> EXPORT_SYMBOL_GPL(insn_rip_relative);
>>> #endif
>>> +
>>> +/**
>>> + *
>>> + * insn_get_length() - Get the length of instruction
>>> + * @insn: &struct insn containing instruction
>>> + *
>>> + * If necessary, first collects the instruction up to and including the
>>> + * ModRM byte.
>>> + */
>> As I mentioned in private email, you or I should probably refactor this
>> into:
>> - insn_get_sib()
>> - insn_get_displacement()
>> - insn_get_immediate()
>> - insn_get_length()
>>
>> BTW, I'm going to have to change my definition of insn_field to
>> accommodate the 8-byte fields that can be found in instructions like
>> a0-a3 (8-byte displacement) and b8-bf (8-byte immediate).
>>
>>> +void insn_get_length(struct insn *insn)
>>> +{
>>> + u8 modrm;
>>> + u8 mod = 0, reg = 0, rm = 0, sib;
>>> + const u8 *next_byte;
>>> + if (insn->length)
>>> + return;
>>> + if (!insn->modrm.got)
>>> + insn_get_modrm(insn);
>>> + next_byte = insn->next_byte;
>> This of course assumes that no fields have been fetched beyond the modrm
>> field.
>>
>>> +
>>> + if (insn->modrm.nbytes) {
>>> + modrm = insn->modrm.value;
>>> + mod = (modrm & 0xc0) >> 6;
>>> + reg = (modrm & 0x38) >> 3;
>>> + rm = (modrm & 0x07);
>> Some comments here would really help -- e.g...
>> /*
>> Interpreting the modrm byte:
>> mod = 00 - no displacement fields (exceptions below)
>> mod = 01 - 1-byte displacement field
>> mod = 10 - displacement field is 4 bytes, or 2 bytes if
>> address size = 2 (0x67 prefix in 32-bit mode)
>> mod = 11 - no memory operand
>>
>> If address size = 2...
>> mod = 00, r/m = 110 - displacement field is 2 bytes
>>
>> If address size != 2...
>> mod != 11, r/m = 100 - SIB byte exists
>> mod = 00, SIB base field = 101 - displacement field is 4 bytes
>> mod = 00, r/m = 101 - rip-relative addressing, displacement
>> field is 4 bytes
>> */
>>
>>> + if (mod == 3)
>>> + goto decode_src;
>>> + if (insn->ad_bytes == 2) {
>>> + if (mod == 1)
>>> + next_byte++;
>>> + else if (mod == 2)
>>> + next_byte += 2;
>>> + else if (rm == 6)
>>> + next_byte += 2;
>>> + } else {
>>> + if (rm == 4) {
>>> + sib = *(next_byte++);
>>> + insn->sib.value = sib;
>>> + insn->sib.nbytes = 1;
>>> + insn->sib.got = 1;
>>> + if ((sib & 7) == 5 && mod == 0)
>>> + next_byte += 4;
>>> + }
>>> + if (mod == 1)
>>> + next_byte++;
>>> + else if (mod == 2)
>>> + next_byte += 4;
>>> + else if (rm == 5)
>>> + next_byte += 4;
>>> + }
>>> + } else if (insn->opcode.nbytes == 1)
>>> + if (0xa0 <= insn->opcode1 && insn->opcode1 < 0xa4)
>> Add comment:
>> /* Displacement = entire address - up to 8 bytes */
>>
>>> + next_byte += insn->ad_bytes;
>>> +decode_src:
>> decode_src is a misnomer. Here we're decoding the immediate operand
>> (which is always a source operand, but not the only kind).
>>
>>> + if (insn->opcode.nbytes == 1) {
>>> + switch (insn->opcode1) {
>>> + case 0x05:
>>> + case 0x25:
>> What about (hex) 15, 35, 01, 0d, 2d?
>>
>>> + case 0x3d:
>>> + case 0x68: // pushl
>>> + case 0x69: // imul
>>> + case 0x9a: /* long call */
>> 0x9a (lcall) seems to have 6 bytes following the opcode, disassembled as
>> 2 immediate operands.
>>
>>> + case 0xa9: // test
>>> + case 0xc7:
>>> + case 0xe8:
>>> + case 0xe9:
>>> + case 0xea: /* long jump */
>> Similarly, 0xea (ljmp) seems to have 6 bytes following the opcode,
>> disassembled as 2 immediate operands.
>>
>>> + case 0x82: /* Group */
>> s/82/81/ here.
>>
>>> + goto imm_common;
>>> + case 0x04:
>>> + case 0x24:
>> What about (hex) 14, 34, 0c, 1c, 2c?
>>
>>> + case 0x3c:
>>> + case 0x6a: //pushb
>>> + case 0x6b: //imul
>>> + case 0xa8: //testb
>>> + case 0xeb:
>>> + case 0xc0:
>>> + case 0xc1:
>>> + case 0xc6:
>>> + case 0x80: /* Group */
>>> + case 0x81: /* Group */
>> s/81/82/ here.
>>
>>> + case 0x83: /* Group */
>>> + goto immbyte_common;
>>> + }
>>> + if ((insn->opcode1 & 0xf8) == 0xb8 ||
>> I don't think this is right. b8-bf can have 8-byte immediate fields
>> (with 0x48 prefix).
>>
>>> + (insn->opcode1 == 0xf7 && reg == 0
>> or reg == 1
>>
>>> ) ) {
>>> +imm_common:
>> Jumping into the middle of an if block is ugly, and not necessary here.
>>
>>> + next_byte += (insn->op_bytes == 8) ? 4 : insn->op_bytes;
>>> + } else if ((insn->opcode1 & 0xf8) == 0xb0 || //
>>> + (insn->opcode1 & 0xf0) == 0x70 || // Jcc
>>> + (insn->opcode1 & 0xf8) == 0xe0 || // loop/in/out
>>> + (insn->opcode1 == 0xf6 && reg == 0)) {
>>> +immbyte_common:
>> Jumping into the middle of an if block is ugly, and not necessary here.
>>
>>> + next_byte++;
>>> + }
>> 0xc8 and 0xcd are weird cases that we should handle .
>>
>>> + } else {
>>> + switch (insn->opcode2) {
>> Add 0x70.
>>
>>> + case 0xa4:
>>> + case 0xac:
>>> + case 0xba:
>>> + case 0x0f: // 3dnow
>>> + case 0x3a: // ssse3
>>> + next_byte++;
>>> + break;
>>> + default:
>>> + if ((insn->opcode2 & 0xf0) == 0x80)
>>> + next_byte += (insn->op_bytes == 8) ? 4 : insn->op_bytes;
>>> + }
>>> + }
>>> + insn->length = (u8)(next_byte - insn->kaddr);
>>> +}
>>> +EXPORT_SYMBOL_GPL(insn_get_length);
>>>
>
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: mhiramat at redhat.com
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: insn_x86.patch
URL: <http://listman.redhat.com/archives/utrace-devel/attachments/20090305/76e70f46/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: insndec.c
URL: <http://listman.redhat.com/archives/utrace-devel/attachments/20090305/76e70f46/attachment.c>
More information about the utrace-devel
mailing list