Hi Borislav, On Wed, 24 Feb 2021 12:02:16 +0100 Borislav Petkov <[email protected]> wrote:
> From: Borislav Petkov <[email protected]> > > Users of the instruction decoder should use this to decode instruction > bytes. For that, have insn*() helpers return an int value to denote > success/failure. When there's an error fetching the next insn byte and > the insn falls short, return -ENODATA to denote that. > > While at it, make insn_get_opcode() more stricter as to whether what has > seen so far is a valid insn and if not. > OK, but I think it should return -EINVAL or -EILSEQ for bad instruction. And I found a bug. [...] > @@ -231,16 +243,25 @@ void insn_get_prefixes(struct insn *insn) > * If necessary, first collects any preceding (prefix) bytes. > * Sets @insn->opcode.value = opcode1. No effect if @insn->opcode.got > * is already 1. > + * > + * Returns: > + * 0: on success > + * < 0: on error OK, but > */ > -void insn_get_opcode(struct insn *insn) > +int insn_get_opcode(struct insn *insn) > { > struct insn_field *opcode = &insn->opcode; > + int pfx_id, ret; > insn_byte_t op; > - int pfx_id; > + > if (opcode->got) > - return; > - if (!insn->prefixes.got) > - insn_get_prefixes(insn); > + return 0; > + > + if (!insn->prefixes.got) { > + ret = insn_get_prefixes(insn); > + if (ret) > + return ret; > + } > > /* Get first opcode */ > op = get_next(insn_byte_t, insn); > @@ -255,9 +276,13 @@ void insn_get_opcode(struct insn *insn) > insn->attr = inat_get_avx_attribute(op, m, p); > if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) || > (!inat_accept_vex(insn->attr) && > - !inat_is_group(insn->attr))) > - insn->attr = 0; /* This instruction is bad */ > - goto end; /* VEX has only 1 byte for opcode */ > + !inat_is_group(insn->attr))) { > + /* This instruction is bad */ > + insn->attr = 0; > + return 1; Here you return 1 for a bad opcode. > + } > + /* VEX has only 1 byte for opcode */ > + goto end; > } > > insn->attr = inat_get_opcode_attribute(op); > @@ -268,13 +293,18 @@ void insn_get_opcode(struct insn *insn) > pfx_id = insn_last_prefix_id(insn); > insn->attr = inat_get_escape_attribute(op, pfx_id, insn->attr); > } > - if (inat_must_vex(insn->attr)) > - insn->attr = 0; /* This instruction is bad */ > + > + if (inat_must_vex(insn->attr)) { > + /* This instruction is bad */ > + insn->attr = 0; > + return 1; Ditto. Would you mean -EINVAL? > + } > end: > opcode->got = 1; > + return 0; > > err_out: > - return; > + return -ENODATA; > } > > /** > @@ -284,15 +314,25 @@ void insn_get_opcode(struct insn *insn) > * Populates @insn->modrm and updates @insn->next_byte to point past the > * ModRM byte, if any. If necessary, first collects the preceding bytes > * (prefixes and opcode(s)). No effect if @insn->modrm.got is already 1. > + * > + * Returns: > + * 0: on success > + * < 0: on error > */ > -void insn_get_modrm(struct insn *insn) > +int insn_get_modrm(struct insn *insn) > { > struct insn_field *modrm = &insn->modrm; > insn_byte_t pfx_id, mod; > + int ret; > + > if (modrm->got) > - return; > - if (!insn->opcode.got) > - insn_get_opcode(insn); > + return 0; > + > + if (!insn->opcode.got) { > + ret = insn_get_opcode(insn); > + if (ret) > + return ret; > + } > > if (inat_has_modrm(insn->attr)) { > mod = get_next(insn_byte_t, insn); > @@ -302,17 +342,22 @@ void insn_get_modrm(struct insn *insn) > pfx_id = insn_last_prefix_id(insn); > insn->attr = inat_get_group_attribute(mod, pfx_id, > insn->attr); > - if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) > - insn->attr = 0; /* This is bad */ > + if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) { > + /* Bad insn */ > + insn->attr = 0; > + return 1; Here is another return 1. > + } > } > } > > if (insn->x86_64 && inat_is_force64(insn->attr)) > insn->opnd_bytes = 8; > + > modrm->got = 1; > + return 0; > > err_out: > - return; > + return -ENODATA; > } > > > @@ -326,11 +371,16 @@ void insn_get_modrm(struct insn *insn) > int insn_rip_relative(struct insn *insn) > { > struct insn_field *modrm = &insn->modrm; > + int ret; > > if (!insn->x86_64) > return 0; > - if (!modrm->got) > - insn_get_modrm(insn); > + > + if (!modrm->got) { > + ret = insn_get_modrm(insn); > + if (ret) > + return ret; > + } > /* > * For rip-relative instructions, the mod field (top 2 bits) > * is zero and the r/m field (bottom 3 bits) is 0x5. > @@ -344,15 +394,25 @@ int insn_rip_relative(struct insn *insn) > * > * If necessary, first collects the instruction up to and including the > * ModRM byte. > + * > + * Returns: > + * 0: if decoding succeeded > + * < 0: otherwise. > */ > -void insn_get_sib(struct insn *insn) > +int insn_get_sib(struct insn *insn) > { > insn_byte_t modrm; > + int ret; > > if (insn->sib.got) > - return; > - if (!insn->modrm.got) > - insn_get_modrm(insn); > + return 0; > + > + if (!insn->modrm.got) { > + ret = insn_get_modrm(insn); > + if (ret) > + return ret; > + } > + > if (insn->modrm.nbytes) { > modrm = (insn_byte_t)insn->modrm.value; > if (insn->addr_bytes != 2 && > @@ -363,8 +423,10 @@ void insn_get_sib(struct insn *insn) > } > insn->sib.got = 1; > > + return 0; > + > err_out: > - return; > + return -ENODATA; > } > > > @@ -375,15 +437,25 @@ void insn_get_sib(struct insn *insn) > * If necessary, first collects the instruction up to and including the > * SIB byte. > * Displacement value is sign-expanded. > + * > + * * Returns: > + * 0: if decoding succeeded > + * < 0: otherwise. > */ > -void insn_get_displacement(struct insn *insn) > +int insn_get_displacement(struct insn *insn) > { > insn_byte_t mod, rm, base; > + int ret; > > if (insn->displacement.got) > - return; > - if (!insn->sib.got) > - insn_get_sib(insn); > + return 0; > + > + if (!insn->sib.got) { > + ret = insn_get_sib(insn); > + if (ret) > + return ret; > + } > + > if (insn->modrm.nbytes) { > /* > * Interpreting the modrm byte: > @@ -426,12 +498,13 @@ void insn_get_displacement(struct insn *insn) > } > out: > insn->displacement.got = 1; > + return 0; > > err_out: > - return; > + return -ENODATA; > } > > -/* Decode moffset16/32/64. Return 0 if failed */ > +/* Decode moffset16/32/64. Return a negative value if failed. */ > static int __get_moffset(struct insn *insn) > { > switch (insn->addr_bytes) { > @@ -457,10 +530,10 @@ static int __get_moffset(struct insn *insn) > return 1; > > err_out: > - return 0; > + return -ENODATA; Also, __get_*() functions are expected to return bool (1/0) for checking bad data. See insn_get_immediate() INAT_IMM_PTR case for example. > } > > -/* Decode imm v32(Iz). Return 0 if failed */ > +/* Decode imm v32(Iz). Return a negative value if failed. */ > static int __get_immv32(struct insn *insn) > { > switch (insn->opnd_bytes) { > @@ -480,10 +553,10 @@ static int __get_immv32(struct insn *insn) > return 1; > > err_out: > - return 0; > + return -ENODATA; Ditto. > } > > -/* Decode imm v64(Iv/Ov), Return 0 if failed */ > +/* Decode imm v64(Iv/Ov). Return a negative value if failed. */ > static int __get_immv(struct insn *insn) > { > switch (insn->opnd_bytes) { > @@ -507,11 +580,12 @@ static int __get_immv(struct insn *insn) > insn->immediate1.got = insn->immediate2.got = 1; > > return 1; > + > err_out: > - return 0; > + return -ENODATA; Ditto. > } > > -/* Decode ptr16:16/32(Ap) */ > +/* Decode ptr16:16/32(Ap). Return a negative value if failed. */ > static int __get_immptr(struct insn *insn) > { > switch (insn->opnd_bytes) { > @@ -534,25 +608,36 @@ static int __get_immptr(struct insn *insn) > insn->immediate1.got = insn->immediate2.got = 1; > > return 1; > + > err_out: > - return 0; > + return -ENODATA; Ditto. Thank you, -- Masami Hiramatsu <[email protected]>
