From 110f797005cca70e18cbcc0975397d26d8045245 Mon Sep 17 00:00:00 2001 From: iximeow Date: Sat, 16 Dec 2023 13:26:48 -0800 Subject: fix opportunity for unhandled register synonyms registers `al`, `cl`, `dl`, and `bl` could have two different representations - with `rex.w` and without. these two forms of `RegSpec` would not compare equal, nor has the same, so for code relying on `RegSpec` to faithfully represent a 1-1 mapping to x86 registers, these synonyms would introduce bugs in register analysis. for example, in `yaxpeax-core`, this would result in instructions writing to `rex.w al` not being visible as definitions for a future read of `!rex.w al`. fix this in `x86_64` code, add new test cases about the confusion, adjust register names to make this situation more clearly a bug, and introduce two new fuzz targets that would have helped spot this error. --- src/long_mode/display.rs | 4 +-- src/long_mode/mod.rs | 68 +++++++++++++++++++++++++++++++++++------------- 2 files changed, 52 insertions(+), 20 deletions(-) (limited to 'src/long_mode') diff --git a/src/long_mode/display.rs b/src/long_mode/display.rs index b1aeee2..9c6795e 100644 --- a/src/long_mode/display.rs +++ b/src/long_mode/display.rs @@ -105,11 +105,11 @@ impl fmt::Display for Segment { // register names are grouped by indices scaled by 16. // xmm, ymm, zmm all get two indices. const REG_NAMES: &[&'static str] = &[ - "", "", "", "", "", "", "", "", + "BUG", "BUG", "BUG", "BUG", "BUG", "BUG", "BUG", "BUG", "al", "cl", "dl", "bl", "ah", "ch", "dh", "bh", "ax", "cx", "dx", "bx", "sp", "bp", "si", "di", "r8w", "r9w", "r10w", "r11w", "r12w", "r13w", "r14w", "r15w", "eax", "ecx", "edx", "ebx", "esp", "ebp", "esi", "edi", "r8d", "r9d", "r10d", "r11d", "r12d", "r13d", "r14d", "r15d", - "al", "cl", "dl", "bl", "spl", "bpl", "sil", "dil", "r8b", "r9b", "r10b", "r11b", "r12b", "r13b", "r14b", "r15b", + "BUG", "BUG", "BUG", "BUG", "spl", "bpl", "sil", "dil", "r8b", "r9b", "r10b", "r11b", "r12b", "r13b", "r14b", "r15b", "rax", "rcx", "rdx", "rbx", "rsp", "rbp", "rsi", "rdi", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15", "cr0", "cr1", "cr2", "cr3", "cr4", "cr5", "cr6", "cr7", "cr8", "cr9", "cr10", "cr11", "cr12", "cr13", "cr14", "cr15", "dr0", "dr1", "dr2", "dr3", "dr4", "dr5", "dr6", "dr7", "dr8", "dr9", "dr10", "dr11", "dr12", "dr13", "dr14", "dr15", diff --git a/src/long_mode/mod.rs b/src/long_mode/mod.rs index 7f9719c..4eb0a36 100644 --- a/src/long_mode/mod.rs +++ b/src/long_mode/mod.rs @@ -883,9 +883,9 @@ pub mod register_class { pub const D: RegisterClass = RegisterClass { kind: RegisterBank::D }; /// word registers: ax through r15w pub const W: RegisterClass = RegisterClass { kind: RegisterBank::W }; - /// byte registers: al, cl, dl, bl, ah, ch, dh, bh. `B` registers do *not* have a rex prefix. + /// byte registers: al, cl, dl, bl, ah, ch, dh, bh. pub const B: RegisterClass = RegisterClass { kind: RegisterBank::B }; - /// byte registers with rex prefix present: al through r15b. `RB` registers have a rex prefix. + /// byte registers added in x86_64: spl through r15b. pub const RB: RegisterClass = RegisterClass { kind: RegisterBank::rB }; /// control registers cr0 through cr15. pub const CR: RegisterClass = RegisterClass { kind: RegisterBank::CR}; @@ -6730,18 +6730,27 @@ fn read_operands< if operand_code.is_only_modrm_operands() { let bank; + let modrm = read_modrm(words)?; + let rrr = (modrm >> 3) & 7; + self.rrr = rrr; + let num = rrr + if instruction.prefixes.rex_unchecked().r() { 0b1000 } else { 0 }; + instruction.regs[0].num = num; + // cool! we can precompute width and know we need to read_E. if !operand_code.has_byte_operands() { // further, this is an vdq E bank = self.vqp_size(); instruction.mem_size = bank as u8; + instruction.regs[0].bank = bank; } else { - bank = self.rb_size(); instruction.mem_size = 1; + bank = self.rb_size(); + if num < 4 { + instruction.regs[0].bank = RegisterBank::B; + } else { + instruction.regs[0].bank = bank; + } }; - let modrm = read_modrm(words)?; - instruction.regs[0].bank = bank; - instruction.regs[0].num = ((modrm >> 3) & 7) + if instruction.prefixes.rex_unchecked().r() { 0b1000 } else { 0 }; // for some encodings, the rrr field selects an opcode, not an operand if operand_code.bits() != OperandCode::ModRM_0xc1_Ev_Ib as u16 && operand_code.bits() != OperandCode::ModRM_0xff_Ev as u16 { @@ -6767,6 +6776,11 @@ fn read_operands< } else { read_M(words, instruction, modrm, sink)? }; + if mem_oper == OperandSpec::RegMMM { + if instruction.regs[1].bank == RegisterBank::rB && instruction.regs[1].num < 4 { + instruction.regs[1].bank = RegisterBank::B; + } + } if !operand_code.has_reg_mem() { instruction.operands[0] = mem_oper; instruction.operands[1] = OperandSpec::RegRRR; @@ -6811,20 +6825,27 @@ fn read_operands< let mut mem_oper = OperandSpec::Nothing; if operand_code.has_read_E() { let bank; + let modrm = read_modrm(words)?; + let rrr = (modrm >> 3) & 7; + self.rrr = rrr; + let num = rrr + if instruction.prefixes.rex_unchecked().r() { 0b1000 } else { 0 }; + instruction.regs[0].num = num; + // cool! we can precompute width and know we need to read_E. if !operand_code.has_byte_operands() { // further, this is an vdq E bank = self.vqp_size(); instruction.mem_size = bank as u8; + instruction.regs[0].bank = bank; } else { - bank = self.rb_size(); instruction.mem_size = 1; + bank = self.rb_size(); + if num < 4 { + instruction.regs[0].bank = RegisterBank::B; + } else { + instruction.regs[0].bank = bank; + } }; - let modrm = read_modrm(words)?; - instruction.regs[0].bank = bank; - let rrr = (modrm >> 3) & 7; - self.rrr = rrr; - instruction.regs[0].num = rrr + if instruction.prefixes.rex_unchecked().r() { 0b1000 } else { 0 }; // for some encodings, the rrr field selects an opcode, not an operand if operand_code.bits() != OperandCode::ModRM_0xc1_Ev_Ib as u16 && operand_code.bits() != OperandCode::ModRM_0xff_Ev as u16 { @@ -6850,6 +6871,11 @@ fn read_operands< } else { read_M(words, instruction, modrm, sink)? }; + if mem_oper == OperandSpec::RegMMM { + if instruction.regs[1].bank == RegisterBank::rB && instruction.regs[1].num < 4 { + instruction.regs[1].bank = RegisterBank::B; + } + } if !operand_code.has_reg_mem() { instruction.operands[0] = mem_oper; instruction.operands[1] = OperandSpec::RegRRR; @@ -6944,11 +6970,13 @@ fn read_operands< } 2 => { // these are Zb_Ib_R - instruction.regs[0] = - RegSpec { - num: reg + if instruction.prefixes.rex_unchecked().b() { 0b1000 } else { 0 }, - bank: self.rb_size() - }; + let num = reg + if instruction.prefixes.rex_unchecked().b() { 0b1000 } else { 0 }; + let bank = if num < 4 { + RegisterBank::B + } else { + self.rb_size() + }; + instruction.regs[0] = RegSpec { num, bank }; sink.record( opcode_start, opcode_start + 2, @@ -7425,7 +7453,11 @@ fn read_operands< ); if instruction.operands[1] == OperandSpec::RegMMM { instruction.mem_size = 0; - instruction.regs[1].bank = w; + if instruction.regs[1].num < 4 { + instruction.regs[1].bank = RegisterBank::B; + } else { + instruction.regs[1].bank = w; + } } else { instruction.mem_size = 1; } -- cgit v1.1