linux-kernel - Re: [PATCH v19 056/130] KVM: x86/tdp_mmu: Init role member of struct kvm_mmu

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zf0wz82nQoL0VsAd@chao-email>
Date: Fri, 22 Mar 2024 15:18:39 +0800
From: Chao Gao <chao.gao@...el.com>
To: Isaku Yamahata <isaku.yamahata@...el.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Zhang, Tina" <tina.zhang@...el.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "Yuan, Hang" <hang.yuan@...el.com>,
	"Huang, Kai" <kai.huang@...el.com>, "Chen, Bo2" <chen.bo@...el.com>,
	"sagis@...gle.com" <sagis@...gle.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, <isaku.yamahata@...ux.intel.com>
Subject: Re: [PATCH v19 056/130] KVM: x86/tdp_mmu: Init role member of struct
 kvm_mmu_page at allocation

On Thu, Mar 21, 2024 at 02:24:12PM -0700, Isaku Yamahata wrote:
>On Thu, Mar 21, 2024 at 12:11:11AM +0000,
>"Edgecombe, Rick P" <rick.p.edgecombe@...el.com> wrote:
>
>> On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@...el.com wrote:
>> > To handle private page tables, argument of is_private needs to be
>> > passed
>> > down.  Given that already page level is passed down, it would be
>> > cumbersome
>> > to add one more parameter about sp. Instead replace the level
>> > argument with
>> > union kvm_mmu_page_role.  Thus the number of argument won't be
>> > increased
>> > and more info about sp can be passed down.
>> > 
>> > For private sp, secure page table will be also allocated in addition
>> > to
>> > struct kvm_mmu_page and page table (spt member).  The allocation
>> > functions
>> > (tdp_mmu_alloc_sp() and __tdp_mmu_alloc_sp_for_split()) need to know
>> > if the
>> > allocation is for the conventional page table or private page table. 
>> > Pass
>> > union kvm_mmu_role to those functions and initialize role member of
>> > struct
>> > kvm_mmu_page.
>> 
>> tdp_mmu_alloc_sp() is only called in two places. One for the root, and
>> one for the mid-level tables.
>> 
>> In later patches when the kvm_mmu_alloc_private_spt() part is added,
>> the root case doesn't need anything done. So the code has to take
>> special care in tdp_mmu_alloc_sp() to avoid doing anything for the
>> root.
>> 
>> It only needs to do the special private spt allocation in non-root
>> case. If we open code that case, I think maybe we could drop this
>> patch, like the below.
>> 
>> The benefits are to drop this patch (which looks to already be part of
>> Paolo's series), and simplify "KVM: x86/mmu: Add a private pointer to
>> struct kvm_mmu_page". I'm not sure though, what do you think? Only
>> build tested.
>
>Makes sense.  Until v18, it had config to disable private mmu part at
>compile time.  Those functions have #ifdef in mmu_internal.h.  v19
>dropped the config for the feedback.
>  https://lore.kernel.org/kvm/Zcrarct88veirZx7@google.com/
>
>After looking at mmu_internal.h, I think the following three function could be
>open coded.
>kvm_mmu_private_spt(), kvm_mmu_init_private_spt(), kvm_mmu_alloc_private_spt(),
>and kvm_mmu_free_private_spt().

It took me a few minutes to figure out why the mirror root page doesn't need
a private_spt.

Per TDX module spec:

  Secure EPT’s root page (EPML4 or EPML5, depending on whether the host VMM uses
  4-level or 5-level EPT) does not need to be explicitly added. It is created
  during TD initialization (TDH.MNG.INIT) and is stored as part of TDCS.

I suggest adding the above as a comment somewhere even if we decide to open-code
kvm_mmu_alloc_private_spt().

IMO, some TDX details bleed into KVM MMU regardless of whether we open-code
kvm_mmu_alloc_private_spt() or not. This isn't good though I cannot think of
a better solution.