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

[Borislav Petkov <bp@alien8.de>]

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.

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.

Do you have a particular reason for why it does that?

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

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.

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...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette