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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201029124231.GB31903@zn.tnic>
Date:   Thu, 29 Oct 2020 13:42:31 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Masami Hiramatsu <mhiramat@...nel.org>,
        Andy Lutomirski <luto@...nel.org>
Cc:     x86-ml <x86@...nel.org>, Joerg Roedel <jroedel@...e.de>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] Have insn decoder functions return success/failure

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