linux-kernel - RE: [PATCH Part2 v6 35/49] KVM: SVM: Remove the long-lived GHCB host map

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:   Fri, 24 Jun 2022 20:14:40 +0000
From:   "Kalra, Ashish" <Ashish.Kalra@....com>
To:     Peter Gonda <pgonda@...gle.com>
CC:     the arch/x86 maintainers <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kvm list <kvm@...r.kernel.org>,
        "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@....com>,
        Borislav Petkov <bp@...en8.de>,
        "Roth, Michael" <Michael.Roth@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Andi Kleen <ak@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>, Marc Orr <marcorr@...gle.com>,
        Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Alper Gun <alpergun@...gle.com>,
        "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        "jarkko@...nel.org" <jarkko@...nel.org>
Subject: RE: [PATCH Part2 v6 35/49] KVM: SVM: Remove the long-lived GHCB host
 map

[AMD Official Use Only - General]

Hello Peter,

>> From: Brijesh Singh <brijesh.singh@....com>
>>
>> On VMGEXIT, sev_handle_vmgexit() creates a host mapping for the GHCB 
>> GPA, and unmaps it just before VM-entry. This long-lived GHCB map is 
>> used by the VMGEXIT handler through accessors such as ghcb_{set_get}_xxx().
>>
>> A long-lived GHCB map can cause issue when SEV-SNP is enabled. When 
>> SEV-SNP is enabled the mapped GPA needs to be protected against a page 
>> state change.
>>
>> To eliminate the long-lived GHCB mapping, update the GHCB sync 
>> operations to explicitly map the GHCB before access and unmap it after 
>> access is complete. This requires that the setting of the GHCBs 
>> sw_exit_info_{1,2} fields be done during sev_es_sync_to_ghcb(), so 
>> create two new fields in the vcpu_svm struct to hold these values when 
>> required to be set outside of the GHCB mapping.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
>> ---
>>  arch/x86/kvm/svm/sev.c | 131 
>> ++++++++++++++++++++++++++---------------
>>  arch/x86/kvm/svm/svm.c |  12 ++--
>>  arch/x86/kvm/svm/svm.h |  24 +++++++-
>>  3 files changed, 111 insertions(+), 56 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 
>> 01ea257e17d6..c70f3f7e06a8 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2823,15 +2823,40 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
>>         kvfree(svm->sev_es.ghcb_sa);
>> }
>>
>> +static inline int svm_map_ghcb(struct vcpu_svm *svm, struct 
>> +kvm_host_map *map) {
>> +       struct vmcb_control_area *control = &svm->vmcb->control;
>> +       u64 gfn = gpa_to_gfn(control->ghcb_gpa);
>> +
>> +       if (kvm_vcpu_map(&svm->vcpu, gfn, map)) {
>> +               /* Unable to map GHCB from guest */
>> +               pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn);
>> +               return -EFAULT;
>> +       }
>> +
>> +       return 0;
>> +}

>There is a perf cost to this suggestion but it might make accessing the GHCB safer for KVM. Have you thought about just using
>kvm_read_guest() or copy_from_user() to fully copy out the GCHB into a KVM owned buffer, then copying it back before the VMRUN. That way the KVM doesn't need to guard against page_state_changes on the GHCBs, that could be a perf ?>improvement in a follow up.

Along with the performance costs you mentioned, the main concern here will be the GHCB write-back path (copying it back) before VMRUN: this will again hit the issue we have currently with 
kvm_write_guest() / copy_to_user(), when we use it to sync the scratch buffer back to GHCB. This can fail if guest RAM is mapped using huge-page(s) and RMP is 4K. Please refer to the patch/fix
mentioned below, kvm_write_guest() potentially can fail before VMRUN in case of SNP :

commit 94ed878c2669532ebae8eb9b4503f19aa33cd7aa
Author: Ashish Kalra <ashish.kalra@....com>
Date:   Mon Jun 6 22:28:01 2022 +0000
        
    KVM: SVM: Sync the GHCB scratch buffer using already mapped ghcb
        
    Using kvm_write_guest() to sync the GHCB scratch buffer can fail
    due to host mapping being 2M, but RMP being 4K. The page fault handling
    in do_user_addr_fault() fails to split the 2M page to handle RMP fault due
    to it being called here in a non-preemptible context. Instead use
    the already kernel mapped ghcb to sync the scratch buffer when the
    scratch buffer is contained within the GHCB.
        
Thanks,
Ashish

>Since we cannot unmap GHCBs I don't think UPM will help here so we probably want to make these patches safe against malicious guests making GHCBs private. But maybe UPM does help?