linux-kernel - Re: [PATCH] x86: svm: don't intercept CR0 TS or MP bit write

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: <20150223191729.GA2186@potion.brq.redhat.com>
Date:	Mon, 23 Feb 2015 20:17:30 +0100
From:	Radim Krčmář <rkrcmar@...hat.com>
To:	Joel Schopp <joel.schopp@....com>
Cc:	Gleb Natapov <gleb@...nel.org>,
	Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
	Joerg Roedel <joro@...tes.org>, Borislav Petkov <bp@...en8.de>,
	linux-kernel@...r.kernel.org, David Kaplan <david.kaplan@....com>
Subject: Re: [PATCH] x86: svm: don't intercept CR0 TS or MP bit write

2015-02-20 16:44-0600, Joel Schopp:
> From: David Kaplan <david.kaplan@....com>
> 
> Reduce the number of exits by avoiding exiting when the guest writes TS or MP
> bits of CR0.  INTERCEPT_CR0_WRITE intercepts all writes to CR0 including TS and
> MP bits. It intercepts these even if INTERCEPT_SELECTIVE_CR0 is set.  What we
> should be doing is setting INTERCEPT_SELECTIVE_CR0 and not setting
> INTERCEPT_CR0_WRITE.
> 
> Signed-off-by: David Kaplan <david.kaplan@....com>
> [added remove of clr_cr_intercept in init_vmcb, fixed check in handle_exit,
> added emulation on interception back in, forward ported, tested]
> Signed-off-by: Joel Schopp <joel.schopp@....com>
> ---
>  arch/x86/kvm/svm.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index d319e0c..55822e5 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1539,10 +1538,8 @@ static void update_cr0_intercept(struct vcpu_svm *svm)
>  
>  	if (gcr0 == *hcr0 && svm->vcpu.fpu_active) {
>  		clr_cr_intercept(svm, INTERCEPT_CR0_READ);


> -		clr_cr_intercept(svm, INTERCEPT_CR0_WRITE);
>  	} else {
>  		set_cr_intercept(svm, INTERCEPT_CR0_READ);

(There is no point in checking fpu_active if cr0s are equal.)

> -		set_cr_intercept(svm, INTERCEPT_CR0_WRITE);

KVM uses lazy FPU and the state is undefined before the first access.
We set cr0.ts when !svm->vcpu.fpu_active to detect the first access, but
if we allow the guest to clear cr0.ts without exiting, it can access FPU
with undefined state.

Is this code failing to disable the intercept when FPU is active?

> @@ -2940,7 +2937,11 @@ static int cr_interception(struct vcpu_svm *svm)
> +	if (svm->vmcb->control.exit_code == SVM_EXIT_CR0_SEL_WRITE)
> +	   cr = 16;
   	^^^

> +	else
> +	   cr = svm->vmcb->control.exit_code - SVM_EXIT_READ_CR0;
   	^^^

Linux uses tabs for indentation.

> @@ -3502,7 +3503,7 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>  	struct kvm_run *kvm_run = vcpu->run;
>  	u32 exit_code = svm->vmcb->control.exit_code;
>  
> -	if (!is_cr_intercept(svm, INTERCEPT_CR0_WRITE))
> +	if (!is_cr_intercept(svm, INTERCEPT_SELECTIVE_CR0))
>  		vcpu->arch.cr0 = svm->vmcb->save.cr0;

I think the purpose of this code is to get changes that happened while
we weren't monitoring CR0, and we introduce a bug here ... MP and TS can
change, but those changes are lost to arch.cr0 now.

(The original code was also suspicious -- we propagate changes of
 vmcb->save.cr0.  I don't think we want to clear guest's CD and NW /
 set MP and TS, which is the result of what we do in svm_set_cr0() /
 update_cr0_intercept() ...)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/