linux-kernel - Re: [V2 PATCH 0/6] KVM: selftests: selftests for fd-based private memory

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: <Y8gpl+LwSuSgBFks@google.com>
Date:   Wed, 18 Jan 2023 17:17:11 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
Cc:     Vishal Annapurve <vannapurve@...gle.com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, pbonzini@...hat.com,
        vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
        joro@...tes.org, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
        shuah@...nel.org, yang.zhong@...el.com, ricarkol@...gle.com,
        aaronlewis@...gle.com, wei.w.wang@...el.com,
        kirill.shutemov@...ux.intel.com, corbet@....net, hughd@...gle.com,
        jlayton@...nel.org, bfields@...ldses.org,
        akpm@...ux-foundation.org, chao.p.peng@...ux.intel.com,
        yu.c.zhang@...ux.intel.com, jun.nakajima@...el.com,
        dave.hansen@...el.com, michael.roth@....com, qperret@...gle.com,
        steven.price@....com, ak@...ux.intel.com, david@...hat.com,
        luto@...nel.org, vbabka@...e.cz, marcorr@...gle.com,
        erdemaktas@...gle.com, pgonda@...gle.com, nikunj@....com,
        diviness@...gle.com, maz@...nel.org, dmatlack@...gle.com,
        axelrasmussen@...gle.com, maciej.szmigiero@...cle.com,
        mizhang@...gle.com, bgardon@...gle.com, ackerleytng@...gle.com
Subject: Re: [V2 PATCH 0/6] KVM: selftests: selftests for fd-based private
 memory

On Wed, Jan 18, 2023, Kirill A. Shutemov wrote:
> On Wed, Jan 18, 2023 at 01:09:49AM +0000, Sean Christopherson wrote:
> > On Mon, Dec 05, 2022, Vishal Annapurve wrote:
> > > This series implements selftests targeting the feature floated by Chao via:
> > > https://lore.kernel.org/lkml/20221202061347.1070246-10-chao.p.peng@linux.intel.com/T/
> > > 
> > > Below changes aim to test the fd based approach for guest private memory
> > > in context of normal (non-confidential) VMs executing on non-confidential
> > > platforms.
> > > 
> > > private_mem_test.c file adds selftest to access private memory from the
> > > guest via private/shared accesses and checking if the contents can be
> > > leaked to/accessed by vmm via shared memory view before/after conversions.
> > > 
> > > Updates in V2:
> > > 1) Simplified vcpu run loop implementation API
> > > 2) Removed VM creation logic from private mem library
> > 
> > I pushed a rework version of this series to:
> > 
> >   git@...hub.com:sean-jc/linux.git x86/upm_base_support
> 
> It still has build issue with lockdep enabled that I mentioned before:

Yeah, I haven't updated the branch since last Friday, i.e. I haven't fixed the
known bugs yet.  I pushed the selftests changes at the same as everything else,
just didn't get to typing up the emails until yesterday.

> https://lore.kernel.org/all/20230116134845.vboraky2nd56szos@box.shutemov.name/
> 
> And when compiled with lockdep, it triggers a lot of warnings about missed
> locks, like this:

Ah crud, I knew I was forgetting something.  The lockdep assertion can simply be
removed, mmu_lock doesn't need to be held to read attributes.  I knew the assertion
was wrong when I added it, but I wanted to remind myself to take a closer look at
the usage of kvm_mem_is_private() and forgot to go back and delete the assertion.

The use of kvm_mem_is_private() in kvm_mmu_do_page_fault() is technically unsafe.
Similar to getting the pfn, if mmu_lock isn't held, consuming the attributes
(private vs. shared) needs MMU notifier protection, i.e. the attributes are safe
to read only after mmu_invalidate_seq is snapshot.

However, is_private gets rechecked by __kvm_faultin_pfn(), which is protected by
the sequence counter, and so the technically unsafe read is verified in the end.
The obvious alternative is to make is_private an output of kvm_faultin_pfn(), but
that's incorrect for SNP and TDX guests, in which case "is_private" is a property
of the fault itself.

TL;DR: I'm going to delete the assertion and add a comment in kvm_mmu_do_page_fault()
explaining why it's safe to consume kvm_mem_is_private() for "legacy" guests.

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 35a339891aed..da0afe81cf10 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2310,8 +2310,6 @@ static inline void kvm_account_pgtable_pages(void *virt, int nr)
 #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
 static inline unsigned long kvm_get_memory_attributes(struct kvm *kvm, gfn_t gfn)
 {
-       lockdep_assert_held(kvm->mmu_lock);
-
        return xa_to_value(xa_load(&kvm->mem_attr_array, gfn));
 }