Re: [RFC] Have insn decoder functions return success/failure

[Masami Hiramatsu]

Hi,

On Thu, 29 Oct 2020 13:42:31 +0100
Borislav Petkov <bp@xxxxxxxxx> 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 <mhiramat@xxxxxxxxxx>