Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 13 Oct 2021 14:34:37 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Brijesh Singh <brijesh.singh@amd.com>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-coco@lists.linux.dev, linux-mm@kvack.org,
        linux-crypto@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Ard Biesheuvel <ardb@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        David Rientjes <rientjes@google.com>,
        Dov Murik <dovmurik@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@ibm.com>,
        Borislav Petkov <bp@alien8.de>,
        Michael Roth <michael.roth@amd.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Andi Kleen <ak@linux.intel.com>, tony.luck@intel.com,
        marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com
Subject: Re: [PATCH Part2 v5 26/45] KVM: SVM: Mark the private vma unmerable
 for SEV-SNP guests
Message-ID: <YWbufTl2CKwJ2uzw@google.com>
References: <20210820155918.7518-1-brijesh.singh@amd.com>
 <20210820155918.7518-27-brijesh.singh@amd.com>
 <YWXYIWuK2T8Kejng@google.com>
 <2a8bf18e-1413-f884-15c4-0927f34ee3b9@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2a8bf18e-1413-f884-15c4-0927f34ee3b9@amd.com>
Precedence: bulk

On Wed, Oct 13, 2021, Brijesh Singh wrote:
> 
> On 10/12/21 11:46 AM, Sean Christopherson wrote:
> > On Fri, Aug 20, 2021, Brijesh Singh wrote:
> >> When SEV-SNP is enabled, the guest private pages are added in the RMP
> >> table; while adding the pages, the rmp_make_private() unmaps the pages
> >> from the direct map. If KSM attempts to access those unmapped pages then
> >> it will trigger #PF (page-not-present).
> >>
> >> Encrypted guest pages cannot be shared between the process, so an
> >> userspace should not mark the region mergeable but to be safe, mark the
> >> process vma unmerable before adding the pages in the RMP table.
> > To be safe from what?  Does the !PRESENT #PF crash the kernel?
> 
> Yes, kernel crashes when KSM attempts to access to an unmaped pfn.

Is this problem unique to nuking the direct map (patch 05), or would it also be
a problem (in the form of an RMP violation) if the direct map were demoted to 4k
pages?
 
> [...]
> >> +	mmap_write_lock(kvm->mm);
> >> +	ret = snp_mark_unmergable(kvm, params.uaddr, params.len);
> >> +	mmap_write_unlock(kvm->mm);
> > This does not, and practically speaking cannot, work.  There are multiple TOCTOU
> > bugs, here and in __snp_handle_page_state_change().  Userspace can madvise() the
> > range at any later point, munmap()/mmap() the entire range, mess with the memslots
> > in the PSC case, and so on and so forth.  Relying on MADV_UNMERGEABLE for functional
> > correctness simply cannot work in KVM, barring mmu_notifier and a big pile of code.
> 
> AFAICT, ksm does not exclude the unmapped pfn from its scan list. We
> need to tell ksm somehow to exclude the unmapped pfn from its scan list.
> I understand that if userspace is messing with us, we have an issue, but
> it's a userspace bug ;) To fix it right, we need to enhance ksm to
> exclude the pfn when it is getting unmapped from the direct map. I
> believe that work can be done outside of the SNP series. I am okay to
> drop snp_mark_unmerable(), and until then, we just run with KSM
> disabled. Thoughts?
> 
> thanks