| 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
| ||
|
Message-ID: <YoLpY9MFxNTBidqB@google.com>
Date: Tue, 17 May 2022 00:16:35 +0000
From: David Matlack <dmatlack@...gle.com>
To: Lai Jiangshan <jiangshanlai@...il.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
Paolo Bonzini <pbonzini@...hat.com>,
Sean Christopherson <seanjc@...gle.com>,
Lai Jiangshan <jiangshan.ljs@...group.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH V2 4/7] KVM: X86/MMU: Activate special shadow pages and
remove old logic
On Tue, May 03, 2022 at 11:07:32PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@...group.com>
>
> Activate special shadow pages by allocate special shadow pages in
> mmu_alloc_direct_roots() and mmu_alloc_shadow_roots().
>
> Make shadow walkings walk from the topmost shadow page even it is
> special shadow page so that they can be walked like normal root
> and shadowed PDPTEs can be made and installed on-demand.
>
> Walking from the topmost causes FNAME(fetch) needs to visit high level
> special shadow pages and allocate special shadow pages when shadowing
> NPT for 32bit L1 in 64bit host, so change FNAME(fetch) and
> FNAME(walk_addr_generic) to handle it for affected code.
>
> Do sync from the topmost in kvm_mmu_sync_roots() and simplifies
> the code.
>
> Now all the root pages and pagetable pointed by a present spte in
> struct kvm_mmu are associated by struct kvm_mmu_page, and
> to_shadow_page() is guaranteed to be not NULL.
>
> Affect cases are those that using_special_root_page() return true.
>
> Signed-off-by: Lai Jiangshan <jiangshan.ljs@...group.com>
> ---
> arch/x86/kvm/mmu/mmu.c | 168 +++------------------------------
> arch/x86/kvm/mmu/paging_tmpl.h | 14 ++-
> 2 files changed, 24 insertions(+), 158 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 3fe70ad3bda2..6f626d7e8ebb 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2214,26 +2214,6 @@ static void shadow_walk_init_using_root(struct kvm_shadow_walk_iterator *iterato
> iterator->addr = addr;
> iterator->shadow_addr = root;
> iterator->level = vcpu->arch.mmu->root_role.level;
> -
> - if (iterator->level >= PT64_ROOT_4LEVEL &&
> - vcpu->arch.mmu->cpu_role.base.level < PT64_ROOT_4LEVEL &&
> - !vcpu->arch.mmu->root_role.direct)
> - iterator->level = PT32E_ROOT_LEVEL;
> -
> - if (iterator->level == PT32E_ROOT_LEVEL) {
> - /*
> - * prev_root is currently only used for 64-bit hosts. So only
> - * the active root_hpa is valid here.
> - */
> - BUG_ON(root != vcpu->arch.mmu->root.hpa);
> -
> - iterator->shadow_addr
> - = vcpu->arch.mmu->pae_root[(addr >> 30) & 3];
> - iterator->shadow_addr &= PT64_BASE_ADDR_MASK;
> - --iterator->level;
> - if (!iterator->shadow_addr)
> - iterator->level = 0;
> - }
> }
>
> static void shadow_walk_init(struct kvm_shadow_walk_iterator *iterator,
> @@ -3412,21 +3392,10 @@ void kvm_mmu_free_roots(struct kvm *kvm, struct kvm_mmu *mmu,
> &invalid_list);
>
> if (free_active_root) {
> - if (to_shadow_page(mmu->root.hpa)) {
> - if (using_special_root_page(mmu))
> - mmu_free_special_root_page(kvm, mmu);
> - else
> - mmu_free_root_page(kvm, &mmu->root.hpa, &invalid_list);
> - } else if (mmu->pae_root) {
> - for (i = 0; i < 4; ++i) {
> - if (!IS_VALID_PAE_ROOT(mmu->pae_root[i]))
> - continue;
> -
> - mmu_free_root_page(kvm, &mmu->pae_root[i],
> - &invalid_list);
> - mmu->pae_root[i] = INVALID_PAE_ROOT;
> - }
> - }
> + if (using_special_root_page(mmu))
> + mmu_free_special_root_page(kvm, mmu);
> + else
> + mmu_free_root_page(kvm, &mmu->root.hpa, &invalid_list);
> mmu->root.hpa = INVALID_PAGE;
> mmu->root.pgd = 0;
> }
> @@ -3491,7 +3460,6 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
> struct kvm_mmu *mmu = vcpu->arch.mmu;
> u8 shadow_root_level = mmu->root_role.level;
> hpa_t root;
> - unsigned i;
> int r;
>
> write_lock(&vcpu->kvm->mmu_lock);
> @@ -3502,24 +3470,9 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
> if (is_tdp_mmu_enabled(vcpu->kvm)) {
> root = kvm_tdp_mmu_get_vcpu_root_hpa(vcpu);
> mmu->root.hpa = root;
> - } else if (shadow_root_level >= PT64_ROOT_4LEVEL) {
> + } else if (shadow_root_level >= PT32E_ROOT_LEVEL) {
> root = mmu_alloc_root(vcpu, 0, 0, shadow_root_level, true);
> mmu->root.hpa = root;
> - } else if (shadow_root_level == PT32E_ROOT_LEVEL) {
> - if (WARN_ON_ONCE(!mmu->pae_root)) {
> - r = -EIO;
> - goto out_unlock;
> - }
> -
> - for (i = 0; i < 4; ++i) {
> - WARN_ON_ONCE(IS_VALID_PAE_ROOT(mmu->pae_root[i]));
> -
> - root = mmu_alloc_root(vcpu, i << (30 - PAGE_SHIFT),
> - i << 30, PT32_ROOT_LEVEL, true);
> - mmu->pae_root[i] = root | PT_PRESENT_MASK |
> - shadow_me_mask;
> - }
> - mmu->root.hpa = __pa(mmu->pae_root);
> } else {
> WARN_ONCE(1, "Bad TDP root level = %d\n", shadow_root_level);
> r = -EIO;
> @@ -3597,10 +3550,8 @@ static int mmu_first_shadow_root_alloc(struct kvm *kvm)
> static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
> {
> struct kvm_mmu *mmu = vcpu->arch.mmu;
> - u64 pdptrs[4], pm_mask;
> gfn_t root_gfn, root_pgd;
> hpa_t root;
> - unsigned i;
> int r;
>
> root_pgd = mmu->get_guest_pgd(vcpu);
> @@ -3609,21 +3560,6 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
> if (mmu_check_root(vcpu, root_gfn))
> return 1;
>
> - /*
> - * On SVM, reading PDPTRs might access guest memory, which might fault
> - * and thus might sleep. Grab the PDPTRs before acquiring mmu_lock.
> - */
> - if (mmu->cpu_role.base.level == PT32E_ROOT_LEVEL) {
> - for (i = 0; i < 4; ++i) {
> - pdptrs[i] = mmu->get_pdptr(vcpu, i);
> - if (!(pdptrs[i] & PT_PRESENT_MASK))
> - continue;
> -
> - if (mmu_check_root(vcpu, pdptrs[i] >> PAGE_SHIFT))
> - return 1;
> - }
> - }
> -
> r = mmu_first_shadow_root_alloc(vcpu->kvm);
> if (r)
> return r;
> @@ -3633,70 +3569,9 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
> if (r < 0)
> goto out_unlock;
>
> - /*
> - * Do we shadow a long mode page table? If so we need to
> - * write-protect the guests page table root.
> - */
> - if (mmu->cpu_role.base.level >= PT64_ROOT_4LEVEL) {
> - root = mmu_alloc_root(vcpu, root_gfn, 0,
> - mmu->root_role.level, false);
> - mmu->root.hpa = root;
> - goto set_root_pgd;
> - }
> -
> - if (WARN_ON_ONCE(!mmu->pae_root)) {
> - r = -EIO;
> - goto out_unlock;
> - }
> -
> - /*
> - * We shadow a 32 bit page table. This may be a legacy 2-level
> - * or a PAE 3-level page table. In either case we need to be aware that
> - * the shadow page table may be a PAE or a long mode page table.
> - */
> - pm_mask = PT_PRESENT_MASK | shadow_me_value;
> - if (mmu->root_role.level >= PT64_ROOT_4LEVEL) {
> - pm_mask |= PT_ACCESSED_MASK | PT_WRITABLE_MASK | PT_USER_MASK;
> -
> - if (WARN_ON_ONCE(!mmu->pml4_root)) {
> - r = -EIO;
> - goto out_unlock;
> - }
> - mmu->pml4_root[0] = __pa(mmu->pae_root) | pm_mask;
> -
> - if (mmu->root_role.level == PT64_ROOT_5LEVEL) {
> - if (WARN_ON_ONCE(!mmu->pml5_root)) {
> - r = -EIO;
> - goto out_unlock;
> - }
> - mmu->pml5_root[0] = __pa(mmu->pml4_root) | pm_mask;
> - }
> - }
> -
> - for (i = 0; i < 4; ++i) {
> - WARN_ON_ONCE(IS_VALID_PAE_ROOT(mmu->pae_root[i]));
> -
> - if (mmu->cpu_role.base.level == PT32E_ROOT_LEVEL) {
> - if (!(pdptrs[i] & PT_PRESENT_MASK)) {
> - mmu->pae_root[i] = INVALID_PAE_ROOT;
> - continue;
> - }
> - root_gfn = pdptrs[i] >> PAGE_SHIFT;
> - }
> -
> - root = mmu_alloc_root(vcpu, root_gfn, i << 30,
> - PT32_ROOT_LEVEL, false);
> - mmu->pae_root[i] = root | pm_mask;
> - }
> -
> - if (mmu->root_role.level == PT64_ROOT_5LEVEL)
> - mmu->root.hpa = __pa(mmu->pml5_root);
> - else if (mmu->root_role.level == PT64_ROOT_4LEVEL)
> - mmu->root.hpa = __pa(mmu->pml4_root);
> - else
> - mmu->root.hpa = __pa(mmu->pae_root);
> -
> -set_root_pgd:
> + root = mmu_alloc_root(vcpu, root_gfn, 0,
> + mmu->root_role.level, false);
> + mmu->root.hpa = root;
> mmu->root.pgd = root_pgd;
> out_unlock:
> write_unlock(&vcpu->kvm->mmu_lock);
> @@ -3813,8 +3688,7 @@ static bool is_unsync_root(hpa_t root)
>
> void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu)
> {
> - int i;
> - struct kvm_mmu_page *sp;
> + hpa_t root = vcpu->arch.mmu->root.hpa;
>
> if (vcpu->arch.mmu->root_role.direct)
> return;
> @@ -3824,31 +3698,11 @@ void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu)
>
> vcpu_clear_mmio_info(vcpu, MMIO_GVA_ANY);
>
> - if (vcpu->arch.mmu->cpu_role.base.level >= PT64_ROOT_4LEVEL) {
> - hpa_t root = vcpu->arch.mmu->root.hpa;
> - sp = to_shadow_page(root);
> -
> - if (!is_unsync_root(root))
> - return;
> -
> - write_lock(&vcpu->kvm->mmu_lock);
> - mmu_sync_children(vcpu, sp, true);
> - write_unlock(&vcpu->kvm->mmu_lock);
> + if (!is_unsync_root(root))
> return;
> - }
>
> write_lock(&vcpu->kvm->mmu_lock);
> -
> - for (i = 0; i < 4; ++i) {
> - hpa_t root = vcpu->arch.mmu->pae_root[i];
> -
> - if (IS_VALID_PAE_ROOT(root)) {
> - root &= PT64_BASE_ADDR_MASK;
> - sp = to_shadow_page(root);
> - mmu_sync_children(vcpu, sp, true);
> - }
> - }
> -
> + mmu_sync_children(vcpu, to_shadow_page(root), true);
> write_unlock(&vcpu->kvm->mmu_lock);
> }
>
> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index b025decf610d..19ef31a078fa 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -316,6 +316,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
> u16 errcode = 0;
> gpa_t real_gpa;
> gfn_t gfn;
> + int i;
>
> trace_kvm_mmu_pagetable_walk(addr, access);
> retry_walk:
> @@ -323,6 +324,16 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
> pte = mmu->get_guest_pgd(vcpu);
> have_ad = PT_HAVE_ACCESSED_DIRTY(mmu);
>
> + /*
> + * FNAME(fetch) might pass these values to allocate special shadow
> + * page. Although the gfn is not used at the end, it is better not
> + * to pass an uninitialized value to kvm_mmu_get_page().
> + */
This comment does not explain why FNAME(fetch) might pass these values
to allocate special shadow pages. How about adjust it to something like
this:
/*
* Initialize the guest walker with default values. These values will
* be used in cases where KVM shadows a guest page table structure
* with more levels than what the guest. For example, KVM shadows
* 2-level non-PAE guests with 3-level PAE paging.
*
* Note, the gfn is technically ignored for these special shadow
* pages, but it's more consistent to always pass 0 to
* kvm_mmu_get_page().
*/
> + for (i = 2; i < PT_MAX_FULL_LEVELS; i++) {
s/2/PT32_ROOT_LEVEL/
> + walker->table_gfn[i] = 0;
> + walker->pt_access[i] = ACC_ALL;
> + }
> +
> #if PTTYPE == 64
> walk_nx_mask = 1ULL << PT64_NX_SHIFT;
> if (walker->level == PT32E_ROOT_LEVEL) {
> @@ -675,7 +686,8 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
> * Verify that the gpte in the page we've just write
> * protected is still there.
> */
> - if (FNAME(gpte_changed)(vcpu, gw, it.level - 1))
> + if (it.level - 1 < top_level &&
> + FNAME(gpte_changed)(vcpu, gw, it.level - 1))
> goto out_gpte_changed;
>
> if (sp)
> --
> 2.19.1.6.gb485710b
>
Powered by blists - more mailing lists