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, 8 Jan 2024 10:40:11 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Borislav Petkov <bp@...en8.de>, X86 ML <x86@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/sev: Harden #VC insn emulation somewhat

On 1/5/24 04:14, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@...en8.de>
> 
> Compare the opcode bytes at rIP for each #VC exit reason to verify the
> instruction which raised the #VC exception is actually the right one.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@...en8.de>

One minor comment below that is up to you. Either way:

Acked-by: Tom Lendacky <thomas.lendacky@....com>

> ---
>   arch/x86/boot/compressed/sev.c |   4 ++
>   arch/x86/kernel/sev-shared.c   | 102 ++++++++++++++++++++++++++++++++-
>   arch/x86/kernel/sev.c          |   9 ++-
>   3 files changed, 108 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 454acd7a2daf..073291832f44 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -304,6 +304,10 @@ void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
>   	if (result != ES_OK)
>   		goto finish;
>   
> +	result = vc_check_opcode_bytes(&ctxt, exit_code);
> +	if (result != ES_OK)
> +		goto finish;
> +
>   	switch (exit_code) {
>   	case SVM_EXIT_RDTSC:
>   	case SVM_EXIT_RDTSCP:
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index ccb0915e84e1..ec17931bf3cd 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -10,11 +10,15 @@
>    */
>   
>   #ifndef __BOOT_COMPRESSED
> -#define error(v)	pr_err(v)
> -#define has_cpuflag(f)	boot_cpu_has(f)
> +#define error(v)			pr_err(v)
> +#define has_cpuflag(f)			boot_cpu_has(f)
> +#define sev_printk(fmt, ...)		printk(fmt, ##__VA_ARGS__)
> +#define sev_printk_rtl(fmt, ...)	printk_ratelimited(fmt, ##__VA_ARGS__)
>   #else
>   #undef WARN
>   #define WARN(condition, format...) (!!(condition))
> +#define sev_printk(fmt, ...)
> +#define sev_printk_rtl(fmt, ...)
>   #endif
>   
>   /* I/O parameters for CPUID-related helpers */
> @@ -574,6 +578,7 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>   {
>   	unsigned int subfn = lower_bits(regs->cx, 32);
>   	unsigned int fn = lower_bits(regs->ax, 32);
> +	u16 opcode = *(unsigned short *)regs->ip;
>   	struct cpuid_leaf leaf;
>   	int ret;
>   
> @@ -581,6 +586,10 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>   	if (exit_code != SVM_EXIT_CPUID)
>   		goto fail;
>   
> +	/* Is it really a CPUID insn? */
> +	if (opcode != 0xa20f)
> +		goto fail;
> +
>   	leaf.fn = fn;
>   	leaf.subfn = subfn;
>   
> @@ -1170,3 +1179,92 @@ static int vmgexit_psc(struct ghcb *ghcb, struct snp_psc_desc *desc)
>   out:
>   	return ret;
>   }
> +
> +static enum es_result vc_check_opcode_bytes(struct es_em_ctxt *ctxt,
> +					    unsigned long exit_code)
> +{
> +	unsigned int opcode = (unsigned int)ctxt->insn.opcode.value;
> +	u8 modrm = ctxt->insn.modrm.value;
> +
> +	switch (exit_code) {
> +
> +	case SVM_EXIT_IOIO:
> +	case SVM_EXIT_NPF:
> +		/* handled separately */
> +		return ES_OK;
> +
> +	case SVM_EXIT_CPUID:
> +		if (opcode == 0xa20f)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_INVD:
> +		sev_printk_rtl(KERN_ERR "#VC exception for INVD??? Seriously???\n");
> +		return ES_UNSUPPORTED;

I think you should actually check the opcode here like the other 
instructions, so that this function is consistent and prints out the error 
message below if it doesn't match.

Then you can just leave the check in vc_handle_exitcode() as originally 
specified.

Thanks,
Tom

> +		break;
> +
> +	case SVM_EXIT_MONITOR:
> +		if (opcode == 0x010f && modrm == 0xc8)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_MWAIT:
> +		if (opcode == 0x010f && modrm == 0xc9)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_MSR:
> +		/* RDMSR */
> +		if (opcode == 0x320f ||
> +		/* WRMSR */
> +		    opcode == 0x300f)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_RDPMC:
> +		if (opcode == 0x330f)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_RDTSC:
> +		if (opcode == 0x310f)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_RDTSCP:
> +		if (opcode == 0x010f && modrm == 0xf9)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_READ_DR7:
> +		if (opcode == 0x210f &&
> +		    X86_MODRM_REG(ctxt->insn.modrm.value) == 7)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_VMMCALL:
> +		if (opcode == 0x010f && modrm == 0xd9)
> +			return ES_OK;
> +
> +		break;
> +
> +	case SVM_EXIT_WRITE_DR7:
> +		if (opcode == 0x230f &&
> +		    X86_MODRM_REG(ctxt->insn.modrm.value) == 7)
> +			return ES_OK;
> +		break;
> +
> +	case SVM_EXIT_WBINVD:
> +		if (opcode == 0x90f)
> +			return ES_OK;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	sev_printk(KERN_ERR "Wrong/unhandled opcode bytes: 0x%x, exit_code: 0x%lx, rIP: 0x%lx\n",
> +		   opcode, exit_code, ctxt->regs->ip);
> +
> +	return ES_UNSUPPORTED;
> +}
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index c67285824e82..541a1f87b4f1 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -1752,7 +1752,10 @@ static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt,
>   					 struct ghcb *ghcb,
>   					 unsigned long exit_code)
>   {
> -	enum es_result result;
> +	enum es_result result = vc_check_opcode_bytes(ctxt, exit_code);
> +
> +	if (result != ES_OK)
> +		return result;
>   
>   	switch (exit_code) {
>   	case SVM_EXIT_READ_DR7:
> @@ -1771,10 +1774,6 @@ static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt,
>   	case SVM_EXIT_RDPMC:
>   		result = vc_handle_rdpmc(ghcb, ctxt);
>   		break;
> -	case SVM_EXIT_INVD:
> -		pr_err_ratelimited("#VC exception for INVD??? Seriously???\n");
> -		result = ES_UNSUPPORTED;
> -		break;
>   	case SVM_EXIT_CPUID:
>   		result = vc_handle_cpuid(ghcb, ctxt);
>   		break;

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ