linux-kernel - Re: [PATCH RFC v9 04/51] KVM: x86: Determine shared/private faults using a configurable mask

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:   Thu, 22 Jun 2023 09:55:22 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
        "michael.roth@....com" <michael.roth@....com>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "tobin@....com" <tobin@....com>,
        "liam.merwick@...cle.com" <liam.merwick@...cle.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Luck, Tony" <tony.luck@...el.com>,
        "jmattson@...gle.com" <jmattson@...gle.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "pgonda@...gle.com" <pgonda@...gle.com>,
        "srinivas.pandruvada@...ux.intel.com" 
        <srinivas.pandruvada@...ux.intel.com>,
        "slp@...hat.com" <slp@...hat.com>,
        "rientjes@...gle.com" <rientjes@...gle.com>,
        "alpergun@...gle.com" <alpergun@...gle.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dovmurik@...ux.ibm.com" <dovmurik@...ux.ibm.com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "Wang, Zhi A" <zhi.a.wang@...el.com>,
        "x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
        "Annapurve, Vishal" <vannapurve@...gle.com>,
        "dgilbert@...hat.com" <dgilbert@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "vkuznets@...hat.com" <vkuznets@...hat.com>,
        "vbabka@...e.cz" <vbabka@...e.cz>,
        "marcorr@...gle.com" <marcorr@...gle.com>,
        "ashish.kalra@....com" <ashish.kalra@....com>,
        "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
        "nikunj.dadhania@....com" <nikunj.dadhania@....com>,
        "Rodel, Jorg" <jroedel@...e.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "kirill@...temov.name" <kirill@...temov.name>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "ardb@...nel.org" <ardb@...nel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH RFC v9 04/51] KVM: x86: Determine shared/private faults
 using a configurable mask


> 
> So if we were to straight-forwardly implement that based on how TDX
> currently handles checking for the shared bit in GPA, paired with how
> SEV-SNP handles checking for private bit in fault flags, it would look
> something like:
> 
>   bool kvm_fault_is_private(kvm, gpa, err)
>   {
>     /* SEV-SNP handling */
>     if (kvm->arch.mmu_private_fault_mask)
>       return !!(err & arch.mmu_private_fault_mask);
> 
>     /* TDX handling */
>     if (kvm->arch.gfn_shared_mask)
>       return !!(gpa & arch.gfn_shared_mask);

The logic of the two are identical.  I think they need to be converged.

Either SEV-SNP should convert the error code private bit to the gfn_shared_mask,
or TDX's shared bit should be converted to some private error bit.

Perhaps converting SEV-SNP makes more sense because if I recall correctly SEV
guest also has a C-bit, correct?

Or, ...

> 
>     return false;
>   }
> 
>   kvm_mmu_do_page_fault(vcpu, gpa, err, ...)
>   {
>     struct kvm_page_fault fault = {
>       ...
>       .is_private = kvm_fault_is_private(vcpu->kvm, gpa, err)

... should we do something like:

	.is_private = static_call(kvm_x86_fault_is_private)(vcpu->kvm, gpa, 
							    err);

?

>     };
> 
>     ...
>   }
> 
> And then arch.mmu_private_fault_mask and arch.gfn_shared_mask would be
> set per-KVM-instance, just like they are now with current SNP and TDX
> patchsets, since stuff like KVM self-test wouldn't be setting those
> masks, so it makes sense to do it per-instance in that regard.
> 
> But that still gets a little awkward for the KVM self-test use-case where
> .is_private should sort of be ignored in favor of whatever the xarray
> reports via kvm_mem_is_private(). 
> 

I must have missed something.  Why does KVM self-test have impact to how does
KVM handles private fault? 

> In your Misc. series I believe you
> handled this by introducing a PFERR_HASATTR_MASK bit so we can determine
> whether existing value of fault->is_private should be
> ignored/overwritten or not.
> 
> So maybe kvm_fault_is_private() needs to return an integer value
> instead, like:
> 
>   enum {
>     KVM_FAULT_VMM_DEFINED,
>     KVM_FAULT_SHARED,
>     KVM_FAULT_PRIVATE,
>   }
> 
>   bool kvm_fault_is_private(kvm, gpa, err)
>   {
>     /* SEV-SNP handling */
>     if (kvm->arch.mmu_private_fault_mask)
>       (err & arch.mmu_private_fault_mask) ? KVM_FAULT_PRIVATE : KVM_FAULT_SHARED
> 
>     /* TDX handling */
>     if (kvm->arch.gfn_shared_mask)
>       (gpa & arch.gfn_shared_mask) ? KVM_FAULT_SHARED : KVM_FAULT_PRIVATE
> 
>     return KVM_FAULT_VMM_DEFINED;
>   }
> 
> And then down in __kvm_faultin_pfn() we do:
> 
>   if (fault->is_private == KVM_FAULT_VMM_DEFINED)
>     fault->is_private = kvm_mem_is_private(vcpu->kvm, fault->gfn);
>   else if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn))
>     return kvm_do_memory_fault_exit(vcpu, fault);
> 
>   if (fault->is_private)
>     return kvm_faultin_pfn_private(vcpu, fault);


What does KVM_FAULT_VMM_DEFINED mean, exactly?

Shouldn't the fault type come from _hardware_?