linux-kernel - Re: [PATCHv3 3/4] x86/insn-eval: Introduce insn_decode

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]

Date:   Mon, 29 Nov 2021 15:29:15 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     "Kirill A. Shutemov" <kirill@...temov.name>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Joerg Roedel <jroedel@...e.de>
Cc:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
        linux-kernel@...r.kernel.org,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCHv3 3/4] x86/insn-eval: Introduce insn_decode_mmio()

On 11/3/21 10:45 AM, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@...temov.name>
> 
> In preparation for sharing MMIO instruction decode between SEV-ES and
> TDX, factor out the common decode into a new insn_decode_mmio() helper.
> 
> For regular virtual machine, MMIO is handled by the VMM and KVM
> emulates instructions that caused MMIO. But, this model doesn't work
> for a secure VMs (like SEV or TDX) as VMM doesn't have access to the
> guest memory and register state. So, for TDX or SEV VMM needs
> assistance in handling MMIO. It induces exception in the guest. Guest
> has to decode the instruction and handle it on its own.
> 
> The code is based on the current SEV MMIO implementation.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@...el.com>
> ---
>   arch/x86/include/asm/insn-eval.h | 12 +++++
>   arch/x86/lib/insn-eval.c         | 84 ++++++++++++++++++++++++++++++++
>   2 files changed, 96 insertions(+)
> 
> diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
> index 5c6869565fb3..010d31726d06 100644
> --- a/arch/x86/include/asm/insn-eval.h
> +++ b/arch/x86/include/asm/insn-eval.h
> @@ -29,4 +29,16 @@ int insn_fetch_from_user_inatomic(struct pt_regs *regs,
>   bool insn_decode_from_regs(struct insn *insn, struct pt_regs *regs,
>   			   unsigned char buf[MAX_INSN_SIZE], int buf_size);
>   
> +enum mmio_type {
> +	MMIO_DECODE_FAILED,
> +	MMIO_WRITE,
> +	MMIO_WRITE_IMM,
> +	MMIO_READ,
> +	MMIO_READ_ZERO_EXTEND,
> +	MMIO_READ_SIGN_EXTEND,
> +	MMIO_MOVS,
> +};
> +
> +enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes);
> +
>   #endif /* _ASM_X86_INSN_EVAL_H */
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 385232a67281..ffd040c1c97b 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -1560,3 +1560,87 @@ bool insn_decode_from_regs(struct insn *insn, struct pt_regs *regs,
>   
>   	return true;
>   }
> +
> +/**
> + * insn_decode_mmio() - Decode a MMIO instruction
> + * @insn:	Structure to store decoded instruction
> + * @bytes:	Returns size of memory operand
> + *
> + * Decodes instruction that used for Memory-mapped I/O.
> + *
> + * Returns:
> + *
> + * Type of the instruction. Size of the memory operand is stored in
> + * @bytes. If decode failed, MMIO_DECODE_FAILED returned.
> + */
> +enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
> +{
> +	int type = MMIO_DECODE_FAILED;

Wouldn't "enum mmio_type" vs "int" be better here ?

Thanks,
Tom

> +
> +	*bytes = 0;
> +
> +	if (insn_get_opcode(insn))
> +		return MMIO_DECODE_FAILED;
> +
> +	switch (insn->opcode.bytes[0]) {
> +	case 0x88: /* MOV m8,r8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0x89: /* MOV m16/m32/m64, r16/m32/m64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_WRITE;
> +		break;
> +
> +	case 0xc6: /* MOV m8, imm8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0xc7: /* MOV m16/m32/m64, imm16/imm32/imm64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_WRITE_IMM;
> +		break;
> +
> +	case 0x8a: /* MOV r8, m8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0x8b: /* MOV r16/r32/r64, m16/m32/m64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_READ;
> +		break;
> +
> +	case 0xa4: /* MOVS m8, m8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0xa5: /* MOVS m16/m32/m64, m16/m32/m64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_MOVS;
> +		break;
> +
> +	case 0x0f: /* Two-byte instruction */
> +		switch (insn->opcode.bytes[1]) {
> +		case 0xb6: /* MOVZX r16/r32/r64, m8 */
> +			*bytes = 1;
> +			fallthrough;
> +		case 0xb7: /* MOVZX r32/r64, m16 */
> +			if (!*bytes)
> +				*bytes = 2;
> +			type = MMIO_READ_ZERO_EXTEND;
> +			break;
> +
> +		case 0xbe: /* MOVSX r16/r32/r64, m8 */
> +			*bytes = 1;
> +			fallthrough;
> +		case 0xbf: /* MOVSX r32/r64, m16 */
> +			if (!*bytes)
> +				*bytes = 2;
> +			type = MMIO_READ_SIGN_EXTEND;
> +			break;
> +		}
> +		break;
> +	}
> +
> +	return type;
> +}
>