linux-kernel - Re: [PATCH v6 13/14] KVM: x86: Introduce new KVM_FEATURE_SEV_LIVE

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>] [day] [month] [year] [list]

Message-ID: <20200402232627.GA25831@ashkalra_ubuntu_server>
Date:   Thu, 2 Apr 2020 23:29:45 +0000
From:   Ashish Kalra <ashish.kalra@....com>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com,
        hpa@...or.com, joro@...tes.org, bp@...e.de,
        Thomas <Thomas.Lendacky@....com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        rientjes@...gle.com, srutherford@...gle.com, luto@...nel.org
Subject: Re: [PATCH v6 13/14] KVM: x86: Introduce new
 KVM_FEATURE_SEV_LIVE_MIGRATION feature & Custom MSR.

Hello Brijesh,
> 
> On Tue, Mar 31, 2020 at 05:13:36PM +0000, Ashish Kalra wrote:
> > Hello Brijesh,
> > 
> > > > Actually this is being done somewhat lazily, after the guest
> > > > enables/activates the live migration feature, it should be fine to do it
> > > > here or it can be moved into sev_map_percpu_data() where the first
> > > > hypercalls are done, in both cases the __bss_decrypted section will be
> > > > marked before the live migration process is initiated.
> > > 
> > > 
> > > IMO, its not okay to do it here or inside sev_map_percpu_data(). So far,
> > > as soon as C-bit state is changed in page table we make a hypercall. It
> > > will be good idea to stick to that approach. I don't see any reason why
> > > we need to make an exception for the __bss_decrypted unless I am missing
> > > something. What will happen if VMM initiate the migration while guest
> > > BIOS is booting?  Are you saying its not supported ?
> > > 
> > 
> > The one thing this will require is checking for KVM para capability 
> > KVM_FEATURE_SEV_LIVE_MIGRATION as part of this code in startup_64(), i 
> > need to verify if i can check for this feature so early in startup code.
> > 
> > I need to check for this capability and do the wrmsrl() here as this
> > will be the 1st hypercall in the guest kernel and i will need to
> > enable live migration feature and hypercall support on the host
> > before making the hypercall.
> > 

I added the KVM para feature capability check here in startup_64(), and
as i thought this does "not" work and also as a side effect disables 
the KVM paravirtualization check and so KVM paravirtualization is not
detected later during kernel boot and all KVM paravirt features remain
disabled.

Digged deeper into this and here's what happens ...

kvm_para_has_feature() calls kvm_arch_para_feature() which in turn calls
kvm_cpuid_base() and this invokes __kvm_cpuid_base(). As the 
"boot_cpu_data" is still not populated/setup, therefore, 
__kvm_cpuid_base() does not detect X86_FEATURE_HYPERVISOR and
also as a side effect sets the variable kvm_cpuid_base == 0.

So as the kvm_para_feature() is not detected in startup_64(), therefore 
the hypercall does not get invoked and also as the side effect of calling
kvm_para_feature() in startup_64(), the static variable "kvm_cpuid_base"
gets set to 0, and later during hypervisor detection (kvm_detect), this
variable's setting causes kvm_detect() to return failure and hence
KVM paravirtualization features don't get enabled for the guest kernel.

So, calling kvm_para_has_feature() so early in startup_64() code is 
not going to work, hence, it is probably best to do the hypercall to mark
__bss_decrypted section as decrypted (lazily) as part of sev_map_percpu_data()
as per my original thought.

Thanks,
Ashish