Re: [PATCH] x86/sev: Harden #VC insn emulation somewhat

[Tom Lendacky]

On 1/5/24 04:14, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@xxxxxxxxx>
>
> 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@xxxxxxxxx>

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

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

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