linux-kernel - Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for TDX shared bit of GPA

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, 16 May 2024 12:12:31 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>
CC: "sagis@...gle.com" <sagis@...gle.com>, "Aktas, Erdem"
	<erdemaktas@...gle.com>, "dmatlack@...gle.com" <dmatlack@...gle.com>, "Zhao,
 Yan Y" <yan.y.zhao@...el.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for
 TDX shared bit of GPA



On 16/05/2024 11:59 am, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 11:44 +1200, Huang, Kai wrote:
>>>
>>> Sorry, still not clear. We need to strip the bit away, so we need to know
>>> what
>>> bit it is. The proposal is to not remember it on struct kvm, so where do we
>>> get
>>> it?
>>
>> The TDX specific code can get it when TDX guest is created.
> 
> The TDX specific code sets it. It knows GPAW/shared bit location.
> 
>>
>>>
>>> Actually, we used to allow it to be selected (via GPAW), but now we could
>>> determine it based on EPT level and MAXPA. So we could possibly recalculate
>>> it
>>> in some helper...
>>>
>>> But it seems you are suggesting to do away with the concept of knowing what
>>> the
>>> shared bit is.
>>
>> What I am suggesting is essentially to replace this
>> kvm_gfn_shared_mask() with some kvm_x86_ops callback (which can just
>> return the shared bit), assuming the common code somehow still need it
>> (e.g., setting up the SPTE for shared mapping, which must include the
>> shared bit to the GPA).
>>
>> The advantage of this we can get rid of the concept of 'gfn_shared_mask'
>> in the MMU common code.  All GFNs referenced in the common code is the
>> actual GFN (w/o the shared bit).
> 
> When it is actually being used as the shared bit instead of as a way to check if
> a guest is a TD, what is the problem? I think the shared_mask serves a real
> (small) purpose, but it is misused for a bunch of other stuff. If we move that
> other stuff to new helpers, the shared mask will still be needed for it's
> original job.
> 
> What is the benefit of the x86_ops over a static inline?

I don't have strong objection if the use of kvm_gfn_shared_mask() is 
contained in smaller areas that truly need it.  Let's discuss in 
relevant patch(es).

However I do think the helpers like below makes no sense (for SEV-SNP):

+static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa)
+{
+	gfn_t mask = kvm_gfn_shared_mask(kvm);
+
+	return mask && !(gpa_to_gfn(gpa) & mask);
+}