linux-kernel - Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32

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]

Message-ID: <58b7aa949cbdfefbb6149608ac0f45c8b25d5640.camel@redhat.com>
Date:   Wed, 30 Sep 2020 17:37:41 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, Jim Mattson <jmattson@...gle.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Ingo Molnar <mingo@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Borislav Petkov <bp@...en8.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Joerg Roedel <joro@...tes.org>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v2 1/1] KVM: x86: fix MSR_IA32_TSC read for nested
 migration

On Thu, 2020-09-24 at 19:33 +0200, Paolo Bonzini wrote:
> On 21/09/20 12:38, Maxim Levitsky wrote:
> > MSR reads/writes should always access the L1 state, since the (nested)
> > hypervisor should intercept all the msrs it wants to adjust, and these
> > that it doesn't should be read by the guest as if the host had read it.
> > 
> > However IA32_TSC is an exception. Even when not intercepted, guest still
> > reads the value + TSC offset.
> > The write however does not take any TSC offset into account.
> > 
> > This is documented in Intel's SDM and seems also to happen on AMD as well.
> > 
> > This creates a problem when userspace wants to read the IA32_TSC value and then
> > write it. (e.g for migration)
> > 
> > In this case it reads L2 value but write is interpreted as an L1 value.
> > To fix this make the userspace initiated reads of IA32_TSC return L1 value
> > as well.
> > 
> > Huge thanks to Dave Gilbert for helping me understand this very confusing
> > semantic of MSR writes.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> >  arch/x86/kvm/x86.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 17f4995e80a7e..ed4314641360e 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3219,9 +3219,21 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  	case MSR_IA32_POWER_CTL:
> >  		msr_info->data = vcpu->arch.msr_ia32_power_ctl;
> >  		break;
> > -	case MSR_IA32_TSC:
> > -		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + vcpu->arch.tsc_offset;
> > +	case MSR_IA32_TSC: {
> > +		/*
> > +		 * Intel SDM states that MSR_IA32_TSC read adds the TSC offset
> > +		 * even when not intercepted. AMD manual doesn't explicitly
> > +		 * state this but appears to behave the same.
> > +		 *
> > +		 * However when userspace wants to read this MSR, we should
> > +		 * return it's real L1 value so that its restore will be correct.
> > +		 */
> > +		u64 tsc_offset = msr_info->host_initiated ? vcpu->arch.l1_tsc_offset :
> > +							    vcpu->arch.tsc_offset;
> > +
> > +		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + tsc_offset;
> >  		break;
> > +	}
> >  	case MSR_MTRRcap:
> >  	case 0x200 ... 0x2ff:
> >  		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
> > 
> 
> Applied the patch as it is doing the sanest possible thing for the
> current semantics of host-initiated accesses.
> 
> Paolo
> 
Thanks!

Best regards,
	Maxim Levitsky