linux-kernel - Re: [PATCH v3 2/5] KVM: x86: invert KVM

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: <ED74106C-ECBB-4FA1-83F9-49ED9FB35019@amd.com>
Date:   Thu, 19 Aug 2021 23:02:00 +0000
From:   "Kalra, Ashish" <Ashish.Kalra@....com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "bp@...en8.de" <bp@...en8.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "joro@...tes.org" <joro@...tes.org>,
        "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        "x86@...nel.org" <x86@...nel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "srutherford@...gle.com" <srutherford@...gle.com>,
        "Singh, Brijesh" <brijesh.singh@....com>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>
Subject: Re: [PATCH v3 2/5] KVM: x86: invert KVM_HYPERCALL to default to
 VMMCALL



> On Aug 20, 2021, at 3:38 AM, Kalra, Ashish <Ashish.Kalra@....com> wrote:
> 
> Hello Sean,
> 
>> On Aug 20, 2021, at 2:15 AM, Sean Christopherson <seanjc@...gle.com> wrote:
>> 
>> Preferred shortlog prefix for KVM guest changes is "x86/kvm".  "KVM: x86" is for
>> host changes.
>> 
>>>> On Tue, Jun 08, 2021, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@....com>
>>> 
>>> KVM hypercall framework relies on alternative framework to patch the
>>> VMCALL -> VMMCALL on AMD platform. If a hypercall is made before
>>> apply_alternative() is called then it defaults to VMCALL. The approach
>>> works fine on non SEV guest. A VMCALL would causes #UD, and hypervisor
>>> will be able to decode the instruction and do the right things. But
>>> when SEV is active, guest memory is encrypted with guest key and
>>> hypervisor will not be able to decode the instruction bytes.
>>> 
>>> So invert KVM_HYPERCALL and X86_FEATURE_VMMCALL to default to VMMCALL
>>> and opt into VMCALL.
>> 
>> The changelog needs to explain why SEV hypercalls need to be made before
>> apply_alternative(), why it's ok to make Intel CPUs take #UDs on the unknown
>> VMMCALL, and why this is not creating the same conundrum for TDX.
> 
> I think it makes more sense to stick to the original approach/patch, i.e., introducing a new private hypercall interface like kvm_sev_hypercall3() and let early paravirtualized kernel code invoke this private hypercall interface wherever required.
> 
> This helps avoiding Intel CPUs taking unnecessary #UDs and also avoid using hacks as below.
> 
> TDX code can introduce similar private hypercall interface for their early para virtualized kernel code if required.

Actually, if we are using this kvm_sev_hypercall3() and not modifying KVM_HYPERCALL() then Intel CPUs avoid unnecessary #UDs and TDX code does not need any new interface. Only early AMD/SEV specific code will use this kvm_sev_hypercall3() interface. TDX code will always work with KVM_HYPERCALL().

Thanks,
Ashish

> 
>> 
>> Actually, I don't think making Intel CPUs take #UDs is acceptable.  This patch
>> breaks Linux on upstream KVM on Intel due a bug in upstream KVM.  KVM attempts
>> to patch the "wrong" hypercall to the "right" hypercall, but stupidly does so
>> via an emulated write.  I.e. KVM honors the guest page table permissions and
>> injects a !WRITABLE #PF on the VMMCALL RIP if the kernel code is mapped RX.
>> 
>> In other words, trusting the VMM to not screw up the #UD is a bad idea.  This also
>> makes documenting the "why does SEV need super early hypercalls" extra important.
>> 
> 
> Makes sense.
> 
> Thanks,
> Ashish
> 
>> This patch doesn't work because X86_FEATURE_VMCALL is a synthetic flag and is
>> only set by VMware paravirt code, which is why the patching doesn't happen as
>> would be expected.  The obvious solution would be to manually set X86_FEATURE_VMCALL
>> where appropriate, but given that defaulting to VMCALL has worked for years,
>> defaulting to VMMCALL makes me nervous, e.g. even if we splatter X86_FEATURE_VMCALL
>> into Intel, Centaur, and Zhaoxin, there's a possibility we'll break existing VMs
>> that run on hypervisors that do something weird with the vendor string.
>> 
>> Rather than look for X86_FEATURE_VMCALL, I think it makes sense to have this be
>> a "pure" inversion, i.e. patch in VMCALL if VMMCALL is not supported, as opposed
>> to patching in VMCALL if VMCALL is supproted.
>> 
>> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
>> index 69299878b200..61641e69cfda 100644
>> --- a/arch/x86/include/asm/kvm_para.h
>> +++ b/arch/x86/include/asm/kvm_para.h
>> @@ -17,7 +17,7 @@ static inline bool kvm_check_and_clear_guest_paused(void)
>> #endif /* CONFIG_KVM_GUEST */
>> 
>> #define KVM_HYPERCALL \
>> -        ALTERNATIVE("vmcall", "vmmcall", X86_FEATURE_VMMCALL)
>> +        ALTERNATIVE("vmmcall", "vmcall", ALT_NOT(X86_FEATURE_VMMCALL))
>> 
>> /* For KVM hypercalls, a three-byte sequence of either the vmcall or the vmmcall
>> * instruction.  The hypervisor may replace it with something else but only the
>> 
>>> Cc: Thomas Gleixner <tglx@...utronix.de>
>>> Cc: Ingo Molnar <mingo@...hat.com>
>>> Cc: "H. Peter Anvin" <hpa@...or.com>
>>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>>> Cc: Joerg Roedel <joro@...tes.org>
>>> Cc: Borislav Petkov <bp@...e.de>
>>> Cc: Tom Lendacky <thomas.lendacky@....com>
>>> Cc: x86@...nel.org
>>> Cc: kvm@...r.kernel.org
>>> Cc: linux-kernel@...r.kernel.org
>> 
>> Suggested-by: Sean Christopherson <seanjc@...gle.com>
>> 
>>> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
>> 
>> Is Brijesh the author?  Co-developed-by for a one-line change would be odd...
>> 
>>> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
>>> ---
>>> arch/x86/include/asm/kvm_para.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
>>> index 69299878b200..0267bebb0b0f 100644
>>> --- a/arch/x86/include/asm/kvm_para.h
>>> +++ b/arch/x86/include/asm/kvm_para.h
>>> @@ -17,7 +17,7 @@ static inline bool kvm_check_and_clear_guest_paused(void)
>>> #endif /* CONFIG_KVM_GUEST */
>>> 
>>> #define KVM_HYPERCALL \
>>> -        ALTERNATIVE("vmcall", "vmmcall", X86_FEATURE_VMMCALL)
>>> +    ALTERNATIVE("vmmcall", "vmcall", X86_FEATURE_VMCALL)
>>> 
>>> /* For KVM hypercalls, a three-byte sequence of either the vmcall or the vmmcall
>>> * instruction.  The hypervisor may replace it with something else but only the
>>> -- 
>>> 2.17.1
>>>