From b4617f790475aa8b23552c07fa0242b8e9ee9fab Mon Sep 17 00:00:00 2001 From: Alan Modra Date: Wed, 26 Apr 2023 10:31:01 +0930 Subject: [PATCH] i386-dis.c UB shift and other tidies 1) i386-dis.c:12055:11: runtime error: left shift of negative value -1 Bit twiddling is best done unsigned, due to UB on overflow of signed expressions. Fix this by using bfd_vma rather than bfd_signed_vma everywhere in i386-dis.c except print_displacement. 2) Return get32s and get16 value in a bfd_vma, reducing the need for temp variables. 3) Introduce get16s and get8s functions to simplify the code. 4) With some optimisation options gcc-13 legitimately complains about a fall-through in OP_I. Fix that. OP_I also doesn't need to use "mask" which was wrong for w_mode anyway. 5) Masking with & 0xffffffff is better than casting to unsigned. We don't know for sure that unsigned int is 32-bit. 6) We also don't know that unsigned char is 8 bits. Mask codep accesses everywhere. I don't expect binutils will work on anything other than an 8-bit char host, but if we are masking codep accesses in some places we might as well be consistent. (Better would be to use stdint.h types more in binutils.) --- opcodes/i386-dis.c | 170 ++++++++++++++++++++------------------------- 1 file changed, 76 insertions(+), 94 deletions(-) diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c index 01e5ba81723..100ec43b189 100644 --- a/opcodes/i386-dis.c +++ b/opcodes/i386-dis.c @@ -47,8 +47,10 @@ static void oappend_with_style (instr_info *, const char *, enum disassembler_style); static void oappend (instr_info *, const char *); static void append_seg (instr_info *); -static bool get32s (instr_info *, bfd_signed_vma *); -static bool get16 (instr_info *, int *); +static bool get32s (instr_info *, bfd_vma *); +static bool get16 (instr_info *, bfd_vma *); +static bool get16s (instr_info *, bfd_vma *); +static bool get8s (instr_info *, bfd_vma *); static void set_op (instr_info *, bfd_vma, bool); static bool OP_E (instr_info *, int, int); @@ -8934,7 +8936,7 @@ ckprefix (instr_info *ins) if (!fetch_code (ins->info, ins->codep + 1)) return ckp_fetch_error; newrex = 0; - switch (*ins->codep) + switch (*ins->codep & 0xff) { /* REX prefixes family. */ case 0x40: @@ -8954,7 +8956,7 @@ ckprefix (instr_info *ins) case 0x4e: case 0x4f: if (ins->address_mode == mode_64bit) - newrex = *ins->codep; + newrex = *ins->codep & 0xff; else return ckp_okay; ins->last_rex_prefix = i; @@ -9034,8 +9036,8 @@ ckprefix (instr_info *ins) /* Rex is ignored when followed by another prefix. */ if (ins->rex) return ckp_bogus; - if (*ins->codep != FWAIT_OPCODE) - ins->all_prefixes[i++] = *ins->codep; + if ((*ins->codep & 0xff) != FWAIT_OPCODE) + ins->all_prefixes[i++] = *ins->codep & 0xff; ins->rex = newrex; ins->codep++; length++; @@ -9266,7 +9268,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins) case USE_3BYTE_TABLE: if (!fetch_code (ins->info, ins->codep + 2)) return &err_opcode; - vindex = *ins->codep++; + vindex = *ins->codep++ & 0xff; dp = &three_byte_table[dp->op[1].bytemode][vindex]; ins->end_codep = ins->codep; if (!fetch_modrm (ins)) @@ -9373,7 +9375,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins) } ins->need_vex = true; ins->codep++; - vindex = *ins->codep++; + vindex = *ins->codep++ & 0xff; dp = &xop_table[vex_table_index][vindex]; ins->end_codep = ins->codep; @@ -9438,7 +9440,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins) } ins->need_vex = true; ins->codep++; - vindex = *ins->codep++; + vindex = *ins->codep++ & 0xff; dp = &vex_table[vex_table_index][vindex]; ins->end_codep = ins->codep; /* There is no MODRM byte for VEX0F 77. */ @@ -9473,7 +9475,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins) } ins->need_vex = true; ins->codep++; - vindex = *ins->codep++; + vindex = *ins->codep++ & 0xff; dp = &vex_table[dp->op[1].bytemode][vindex]; ins->end_codep = ins->codep; /* There is no MODRM byte for VEX 77. */ @@ -9565,7 +9567,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins) ins->need_vex = true; ins->codep++; - vindex = *ins->codep++; + vindex = *ins->codep++ & 0xff; dp = &evex_table[vex_table_index][vindex]; ins->end_codep = ins->codep; if (!fetch_modrm (ins)) @@ -9904,10 +9906,11 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax) goto out; } - ins.two_source_ops = (*ins.codep == 0x62) || (*ins.codep == 0xc8); + ins.two_source_ops = ((*ins.codep & 0xff) == 0x62 + || (*ins.codep & 0xff) == 0xc8); - if (((ins.prefixes & PREFIX_FWAIT) - && ((*ins.codep < 0xd8) || (*ins.codep > 0xdf)))) + if ((ins.prefixes & PREFIX_FWAIT) + && ((*ins.codep & 0xff) < 0xd8 || (*ins.codep & 0xff) > 0xdf)) { /* Handle ins.prefixes before fwait. */ for (i = 0; i < ins.fwait_prefix && ins.all_prefixes[i]; @@ -9919,22 +9922,22 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax) goto out; } - if (*ins.codep == 0x0f) + if ((*ins.codep & 0xff) == 0x0f) { unsigned char threebyte; ins.codep++; if (!fetch_code (info, ins.codep + 1)) goto fetch_error_out; - threebyte = *ins.codep; + threebyte = *ins.codep & 0xff; dp = &dis386_twobyte[threebyte]; ins.need_modrm = twobyte_has_modrm[threebyte]; ins.codep++; } else { - dp = &dis386[*ins.codep]; - ins.need_modrm = onebyte_has_modrm[*ins.codep]; + dp = &dis386[*ins.codep & 0xff]; + ins.need_modrm = onebyte_has_modrm[*ins.codep & 0xff]; ins.codep++; } @@ -10635,7 +10638,7 @@ dofloat (instr_info *ins, int sizeflag) const struct dis386 *dp; unsigned char floatop; - floatop = ins->codep[-1]; + floatop = ins->codep[-1] & 0xff; if (ins->modrm.mod != 3) { @@ -10656,7 +10659,7 @@ dofloat (instr_info *ins, int sizeflag) putop (ins, fgrps[dp->op[0].bytemode][ins->modrm.rm], sizeflag); /* Instruction fnstsw is only one with strange arg. */ - if (floatop == 0xdf && ins->codep[-1] == 0xe0) + if (floatop == 0xdf && (ins->codep[-1] & 0xff) == 0xe0) strcpy (ins->op_out[0], att_names16[0] + ins->intel_syntax); } else @@ -11399,10 +11402,9 @@ print_operand_value (instr_info *ins, bfd_vma disp, { char tmp[30]; - if (ins->address_mode == mode_64bit) - sprintf (tmp, "0x%" PRIx64, (uint64_t) disp); - else - sprintf (tmp, "0x%x", (unsigned int) disp); + if (ins->address_mode != mode_64bit) + disp &= 0xffffffff; + sprintf (tmp, "0x%" PRIx64, (uint64_t) disp); oappend_with_style (ins, tmp, style); } @@ -11944,7 +11946,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag) if ((sizeflag & AFLAG) || ins->address_mode == mode_64bit) { /* 32/64 bit address mode */ - bfd_signed_vma disp = 0; + bfd_vma disp = 0; int havedisp; int havebase; int needindex; @@ -12046,11 +12048,8 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag) } break; case 1: - if (!fetch_code (ins->info, ins->codep + 1)) + if (!get8s (ins, &disp)) return false; - disp = *ins->codep++; - if ((disp & 0x80) != 0) - disp -= 0x100; if (ins->vex.evex && shift > 0) disp <<= shift; break; @@ -12073,7 +12072,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag) { /* Without base nor index registers, zero-extend the lower 32-bit displacement to 64 bits. */ - disp = (unsigned int) disp; + disp &= 0xffffffff; needindex = 1; } needaddr32 = 1; @@ -12162,7 +12161,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag) if (ins->intel_syntax && (disp || ins->modrm.mod != 0 || base == 5)) { - if (!havedisp || disp >= 0) + if (!havedisp || (bfd_signed_vma) disp >= 0) oappend_char (ins, '+'); if (havedisp) print_displacement (ins, disp); @@ -12211,7 +12210,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag) else { /* 16 bit address mode */ - int disp = 0; + bfd_vma disp = 0; ins->used_prefixes |= ins->prefixes & PREFIX_ADDR; switch (ins->modrm.mod) @@ -12220,18 +12219,13 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag) if (ins->modrm.rm == 6) { case 2: - if (!get16 (ins, &disp)) + if (!get16s (ins, &disp)) return false; - if ((disp & 0x8000) != 0) - disp -= 0x10000; } break; case 1: - if (!fetch_code (ins->info, ins->codep + 1)) + if (!get8s (ins, &disp)) return false; - disp = *ins->codep++; - if ((disp & 0x80) != 0) - disp -= 0x100; if (ins->vex.evex && shift > 0) disp <<= shift; break; @@ -12249,7 +12243,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag) if (ins->intel_syntax && (disp || ins->modrm.mod != 0 || ins->modrm.rm == 6)) { - if (disp >= 0) + if ((bfd_signed_vma) disp >= 0) oappend_char (ins, '+'); print_displacement (ins, disp); } @@ -12397,7 +12391,7 @@ get64 (instr_info *ins, uint64_t *res) } static bool -get32 (instr_info *ins, bfd_signed_vma *res) +get32 (instr_info *ins, bfd_vma *res) { if (!fetch_code (ins->info, ins->codep + 4)) return false; @@ -12409,7 +12403,7 @@ get32 (instr_info *ins, bfd_signed_vma *res) } static bool -get32s (instr_info *ins, bfd_signed_vma *res) +get32s (instr_info *ins, bfd_vma *res) { if (!get32 (ins, res)) return false; @@ -12420,12 +12414,30 @@ get32s (instr_info *ins, bfd_signed_vma *res) } static bool -get16 (instr_info *ins, int *res) +get16 (instr_info *ins, bfd_vma *res) { if (!fetch_code (ins->info, ins->codep + 2)) return false; - *res = *ins->codep++ & 0xff; - *res |= (*ins->codep++ & 0xff) << 8; + *res = (bfd_vma) *ins->codep++ & 0xff; + *res |= ((bfd_vma) *ins->codep++ & 0xff) << 8; + return true; +} + +static bool +get16s (instr_info *ins, bfd_vma *res) +{ + if (!get16 (ins, res)) + return false; + *res = (*res ^ 0x8000) - 0x8000; + return true; +} + +static bool +get8s (instr_info *ins, bfd_vma *res) +{ + if (!fetch_code (ins->info, ins->codep + 1)) + return false; + *res = (((bfd_vma) *ins->codep++ & 0xff) ^ 0x80) - 0x80; return true; } @@ -12552,16 +12564,14 @@ OP_IMREG (instr_info *ins, int code, int sizeflag) static bool OP_I (instr_info *ins, int bytemode, int sizeflag) { - bfd_signed_vma op; - bfd_signed_vma mask = -1; + bfd_vma op; switch (bytemode) { case b_mode: if (!fetch_code (ins->info, ins->codep + 1)) return false; - op = *ins->codep++; - mask = 0xff; + op = *ins->codep++ & 0xff; break; case v_mode: USED_REX (REX_W); @@ -12578,17 +12588,13 @@ OP_I (instr_info *ins, int bytemode, int sizeflag) case d_mode: if (!get32 (ins, &op)) return false; - mask = 0xffffffff; } else { - int num; - + /* Fall through. */ case w_mode: - if (!get16 (ins, &num)) + if (!get16 (ins, &op)) return false; - op = num; - mask = 0xfffff; } } break; @@ -12601,7 +12607,6 @@ OP_I (instr_info *ins, int bytemode, int sizeflag) return true; } - op &= mask; oappend_immediate (ins, op); return true; } @@ -12627,17 +12632,14 @@ OP_I64 (instr_info *ins, int bytemode, int sizeflag) static bool OP_sI (instr_info *ins, int bytemode, int sizeflag) { - bfd_signed_vma op; + bfd_vma op; switch (bytemode) { case b_mode: case b_T_mode: - if (!fetch_code (ins->info, ins->codep + 1)) + if (!get8s (ins, &op)) return false; - op = *ins->codep++; - if ((op & 0x80) != 0) - op -= 0x100; if (bytemode == b_T_mode) { if (ins->address_mode != mode_64bit @@ -12665,11 +12667,8 @@ OP_sI (instr_info *ins, int bytemode, int sizeflag) /* The operand-size prefix is overridden by a REX prefix. */ if (!(sizeflag & DFLAG) && !(ins->rex & REX_W)) { - int val; - - if (!get16 (ins, &val)) + if (!get16 (ins, &op)) return false; - op = val; } else if (!get32s (ins, &op)) return false; @@ -12693,11 +12692,8 @@ OP_J (instr_info *ins, int bytemode, int sizeflag) switch (bytemode) { case b_mode: - if (!fetch_code (ins->info, ins->codep + 1)) + if (!get8s (ins, &disp)) return false; - disp = *ins->codep++; - if ((disp & 0x80) != 0) - disp -= 0x100; break; case v_mode: case dqw_mode: @@ -12706,19 +12702,13 @@ OP_J (instr_info *ins, int bytemode, int sizeflag) && ((ins->isa64 == intel64 && bytemode != dqw_mode) || (ins->rex & REX_W)))) { - bfd_signed_vma val; - - if (!get32s (ins, &val)) + if (!get32s (ins, &disp)) return false; - disp = val; } else { - int val; - - if (!get16 (ins, &val)) + if (!get16s (ins, &disp)) return false; - disp = val & 0x8000 ? val - 0x10000 : val; /* In 16bit mode, address is wrapped around at 64k within the same segment. Otherwise, a data16 prefix on a jump instruction means that the pc is masked to 16 bits after @@ -12757,16 +12747,14 @@ OP_SEG (instr_info *ins, int bytemode, int sizeflag) static bool OP_DIR (instr_info *ins, int dummy ATTRIBUTE_UNUSED, int sizeflag) { - int seg, offset, res; + bfd_vma seg, offset; + int res; char scratch[24]; if (sizeflag & DFLAG) { - bfd_signed_vma val; - - if (!get32 (ins, &val)) + if (!get32 (ins, &offset)) return false;; - offset = val; } else if (!get16 (ins, &offset)) return false; @@ -12776,7 +12764,7 @@ OP_DIR (instr_info *ins, int dummy ATTRIBUTE_UNUSED, int sizeflag) res = snprintf (scratch, ARRAY_SIZE (scratch), ins->intel_syntax ? "0x%x:0x%x" : "$0x%x,$0x%x", - seg, offset); + (unsigned) seg, (unsigned) offset); if (res < 0 || (size_t) res >= ARRAY_SIZE (scratch)) abort (); oappend (ins, scratch); @@ -12794,19 +12782,13 @@ OP_OFF (instr_info *ins, int bytemode, int sizeflag) if ((sizeflag & AFLAG) || ins->address_mode == mode_64bit) { - bfd_signed_vma val; - - if (!get32 (ins, &val)) + if (!get32 (ins, &off)) return false; - off = val; } else { - int val; - - if (!get16 (ins, &val)) + if (!get16 (ins, &off)) return false; - off = val; } if (ins->intel_syntax) @@ -12876,7 +12858,7 @@ OP_ESreg (instr_info *ins, int code, int sizeflag) { if (ins->intel_syntax) { - switch (ins->codep[-1]) + switch (ins->codep[-1] & 0xff) { case 0x6d: /* insw/insl */ intel_operand_size (ins, z_mode, sizeflag); @@ -12902,7 +12884,7 @@ OP_DSreg (instr_info *ins, int code, int sizeflag) { if (ins->intel_syntax) { - switch (ins->codep[-1]) + switch (ins->codep[-1] & 0xff) { case 0x6f: /* outsw/outsl */ intel_operand_size (ins, z_mode, sizeflag); @@ -13872,7 +13854,7 @@ OP_REG_VexI4 (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED) if (!fetch_code (ins->info, ins->codep + 1)) return false; - reg = *ins->codep++; + reg = *ins->codep++ & 0xff; if (bytemode != x_mode && bytemode != scalar_mode) abort (); -- 2.30.2