Hi,
On Thu, 29 Oct 2020 13:42:31 +0100
Borislav Petkov <b...@alien8.de> wrote:
> Hi Masami,
>
> On Sat, Oct 24, 2020 at 01:27:41AM +0200, Borislav Petkov wrote:
> > @@ -230,14 +231,20 @@ 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
> > */
> > -void insn_get_opcode(struct insn *insn)
> > +int insn_get_opcode(struct insn *insn)
> > {
> > struct insn_field *opcode = &insn->opcode;
> > insn_byte_t op;
> > int pfx_id;
> > +
> > if (opcode->got)
> > - return;
> > + return 0;
> > +
> > if (!insn->prefixes.got)
> > insn_get_prefixes(insn);
> >
> > @@ -254,9 +261,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;
> > + }
> > + /* VEX has only 1 byte for opcode */
> > + goto end;
>
> so I'm playing more with this and am hitting the following after I made
> this change to insn_get_opcode() to actually return an error because,
> well, it is an error when the opcode bytes are pointing to an invalid
> insn.
OK, let me see.
>
> However, the current situation is that even though the comment says that
> the instruction is bad:
>
> 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 */
>
> it would goto to end and set opcode->got = 1, i.e., denote success.
Ah, it should be a bug.
>
> Do you have a particular reason for why it does that?
No, I think I have made a bug..
>
> Because, for example, when it encounters an invalid VEX insn which is
> bad, running insn_sanity says this:
>
> Error: Found an access violation:
> Instruction = {
> .prefixes = {
> .value = 0, bytes[] = {0, 0, 0, 0},
> .got = 1, .nbytes = 0},
> .rex_prefix = {
> .value = 0, bytes[] = {0, 0, 0, 0},
> .got = 1, .nbytes = 0},
> .vex_prefix = {
> .value = 7138501, bytes[] = {c5, ec, 6c, 0},
> .got = 1, .nbytes = 2},
> .opcode = {
> .value = 149, bytes[] = {95, 0, 0, 0},
> .got = 0, .nbytes = 1},
> .modrm = {
> .value = 0, bytes[] = {0, 0, 0, 0},
> .got = 0, .nbytes = 0},
> .sib = {
> .value = 0, bytes[] = {0, 0, 0, 0},
> .got = 0, .nbytes = 0},
> .displacement = {
> .value = 0, bytes[] = {0, 0, 0, 0},
> .got = 0, .nbytes = 0},
> .immediate1 = {
> .value = 0, bytes[] = {0, 0, 0, 0},
> .got = 0, .nbytes = 0},
> .immediate2 = {
> .value = 0, bytes[] = {0, 0, 0, 0},
> .got = 0, .nbytes = 0},
> .attr = 0, .opnd_bytes = 4, .addr_bytes = 8,
> .length = 0, .x86_64 = 1, .kaddr = 0x7ffe7cc46460}
> You can reproduce this with below command(s);
> $ echo c5 ec 95 b2 02 bd 4b c8 a8 36 b2 c5 c0 df 13 |
> arch/x86/tools/insn_sanity -i -
> Or
> $ arch/x86/tools/insn_sanity -s 0x87ac2160,109
What's the objdump say here?
>
> I do
>
> arch/x86/tools/insn_sanity -s 0x87ac2160 -v -y
>
> After having added debug output, it says:
>
> inat_get_avx_attribute: vex_m: 0x1, vex_p: 0x0
> inat_get_avx_attribute: looking up opcode 0x95
> insn_get_opcode: insn is bad, must_evex: 0, !accept_vex: 1, !is_group: 1
> get_opcode
> get_modrm
> get_sib
> get_displacement
> get_immediate failed
> insn_decode: here
> main: ret: -22
> Error: Found an access violation:
>
> so long story short, 0xc5 0xec 0x95 is an invalid VEX insn because
> there's no VEX insn with opcode 0x95.
Yes.
>
> So it really is a bad insn.
>
> So after my changes, insn_decode() becomes stricter but that would need
> adjusting the sanity checker. And before I do that, let me run it by you
> in case I'm missing some other aspect...
Yes, in this case, we would better to handle it as an undecodable input
instead of access violation in insn_sanity.
Thank you,
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
--
Masami Hiramatsu <mhira...@kernel.org>
