linux-kernel - Re: [PATCH] kvm: implement VEX prefix decoder, bextr/andn

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] [day] [month] [year] [list]

Date:   Fri, 29 Jun 2018 13:47:00 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Mason Lee Back <masonleeback@...il.com>
Cc:     Radim Krčmář <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kvm: implement VEX prefix decoder, bextr/andn

Thanks, I have some comments on the decoding logic.

On 26/06/2018 14:14, Mason Lee Back wrote:
> +	if (ctxt->b == 0xC4 || ctxt->b == 0xC5) {

This should be exactly the condition that you are removing below:

	if ((ctxt->b == 0xc5 || ctxt->b == 0xc4) &&
	    (mode == X86EMUL_MODE_PROT64 || (ctxt->modrm & 0xc0) == 0xc0))

so that you don't need the "reinterpret as LES or LDS" todo.  It should also
be moved earlier, right after done_prefixes.  The existing code starting
at

	/* Opcode byts(s).  */

and ending just before

	ctxt->d = opcode.flags;

will become the "else" branch.

Otherwise looks at least... sane. :)

Paolo

> +		ctxt->vex.prefix = ctxt->b;
> +		if (ctxt->b == 0xC4) {
> +			ctxt->vex.value = insn_fetch(u16, ctxt);
> +		} else {
> +			ctxt->vex.value = insn_fetch(u8, ctxt) << 8;
> +			ctxt->vex.r = ctxt->vex.w;
> +			ctxt->vex.w = 1;
> +			ctxt->vex.x = 1;
> +			ctxt->vex.b = 1;
> +			ctxt->vex.m = 1;
> +		}
>  
> -	/* vex-prefix instructions are not implemented */
> -	if (ctxt->opcode_len == 1 && (ctxt->b == 0xc5 || ctxt->b == 0xc4) &&
> -	    (mode == X86EMUL_MODE_PROT64 || (ctxt->modrm & 0xc0) == 0xc0)) {
> -		ctxt->d = NotImpl;
> +		if (mode != X86EMUL_MODE_PROT64 && (!ctxt->vex.r || !ctxt->vex.x)) {
> +			/* todo: reinterpret as LES (0xC4) or LDS (0xC5) instruction */
> +			return EMULATION_FAILED;
> +		}
> +
> +		ctxt->rex_prefix |= ctxt->vex.r ? 0 : (1 << 2); /* rex.r */
> +		ctxt->rex_prefix |= ctxt->vex.x ? 0 : (1 << 1); /* rex.x */
> +		ctxt->rex_prefix |= ctxt->vex.b ? 0 : (1 << 0); /* rex.b */
> +		if (mode == X86EMUL_MODE_PROT64 && ctxt->vex.w) {
> +			ctxt->op_bytes = 8;
> +		}

Please set ctxt->rex_prefix too for consistency.
> +
> +		ctxt->b = insn_fetch(u8, ctxt);
> +		switch (ctxt->vex.m) {
> +		case 1:
> +			opcode = twobyte_table[ctxt->b];

"ctxt->opcode_len = 2;" missing here.

> +			break;
> +		case 2:
> +			opcode = opcode_map_0f_38[ctxt->b];

"ctxt->opcode_len = 3;" missing here.

> +			break;
> +		case 3:
> +			/* KVM doesn't support this */
> +			return EMULATION_FAILED;
> +		default:
> +			return EMULATION_FAILED;
> +		}
>  	}