Received-SPF: pass (google.com: domain of linux-kernel+bounces-40870-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date: Fri, 26 Jan 2024 17:54:20 -0600
From: Michael Roth <michael.roth@amd.com>
To: Borislav Petkov <bp@alien8.de>
CC: <x86@kernel.org>, <kvm@vger.kernel.org>, <linux-coco@lists.linux.dev>,
	<linux-mm@kvack.org>, <linux-crypto@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <tglx@linutronix.de>, <mingo@redhat.com>,
	<jroedel@suse.de>, <thomas.lendacky@amd.com>, <hpa@zytor.com>,
	<ardb@kernel.org>, <pbonzini@redhat.com>, <seanjc@google.com>,
	<vkuznets@redhat.com>, <jmattson@google.com>, <luto@kernel.org>,
	<dave.hansen@linux.intel.com>, <slp@redhat.com>, <pgonda@google.com>,
	<peterz@infradead.org>, <srinivas.pandruvada@linux.intel.com>,
	<rientjes@google.com>, <tobin@ibm.com>, <vbabka@suse.cz>,
	<kirill@shutemov.name>, <ak@linux.intel.com>, <tony.luck@intel.com>,
	<sathyanarayanan.kuppuswamy@linux.intel.com>, <alpergun@google.com>,
	<jarkko@kernel.org>, <ashish.kalra@amd.com>, <nikunj.dadhania@amd.com>,
	<pankaj.gupta@amd.com>, <liam.merwick@oracle.com>
Subject: Re: [PATCH v2 11/25] x86/sev: Adjust directmap to avoid inadvertant
 RMP faults
Message-ID: <20240126235420.mu644waj2eyoxqx6@amd.com>
References: <20240126041126.1927228-1-michael.roth@amd.com>
 <20240126041126.1927228-12-michael.roth@amd.com>
 <20240126153451.GDZbPRG3KxaQik-0aY@fat_crate.local>
 <20240126170415.f7r4nvsrzgpzcrzv@amd.com>
 <20240126184340.GEZbP9XA13X91-eybA@fat_crate.local>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240126184340.GEZbP9XA13X91-eybA@fat_crate.local>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 23:54:39.9632
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 7e2cd9f6-4175-416d-6f6d-08dc1eca2908
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	MN1PEPF0000ECDB.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6577

On Fri, Jan 26, 2024 at 07:43:40PM +0100, Borislav Petkov wrote:
> On Fri, Jan 26, 2024 at 11:04:15AM -0600, Michael Roth wrote:
> > vaddr comes from pfn_to_kaddr(pfn), i.e. __va(paddr), so it will
> > necessarily be a direct-mapped address above __PAGE_OFFSET.
> 
> Ah, true.
> 
> > For upper-end, a pfn_valid(pfn) check might suffice, since only a valid
> > PFN would have a possibly-valid mapping wthin the directmap range.
> 
> Looking at it, yap, that could be a sensible thing to check.
> 
> > These are PFNs that are owned/allocated-to the caller. Due to the nature
> > of the directmap it's possible non-owners would write to a mapping that
> > overlaps, but vmalloc()/etc. would not create mappings for any pages that
> > were not specifically part of an allocation that belongs to the caller,
> > so I don't see where there's any chance for an overlap there. And the caller
> > of these functions would not be adjusting directmap for PFNs that might be
> > mapped into other kernel address ranges like kernel-text/etc unless the
> > caller was specifically making SNP-aware adjustments to those ranges, in
> > which case it would be responsible for making those other adjustments,
> > or implementing the necessary helpers/etc.
> 
> Why does any of that matter?
> 
> If you can make this helper as generic as possible now, why don't you?

In this case, it would make it more difficult to handle things
efficiently and implement proper bounds-checking/etc. For instance, if
the caller *knows* they are doing something different like splitting a
kernel-text mapping, then we could implement proper bounds-checking
based on expected ranges, and implement any special handling associated
with that use-case, and capture that in a nice/understandable
adjust_kernel_text_mapping() helper. Or maybe if these are adjustments
for non-static/non-linear mappings, it makes more sense to be given an
HVA rather than PFN, etc., since we might not have any sort of reverse-map
structure/function than can be used to do the PFN->HVA lookups efficiently.

It's just a lot to guess at. And just the directmap splitting itself has
been the source of so much discussion, investigation, re-work, benchmarking,
etc., that it hints that implementing similar handling for other use-cases
really needs to have a clear and necessary purpose and set of requirements 
hat can be evaluated/tested before enabling them and reasonably expecting
them to work as expected.

> 
> > I'm not aware of such cases in the current code, and I don't think it makes
> > sense to attempt to try to handle them here generically until such a case
> > arises, since it will likely involve more specific requirements than what
> > we can anticipate from a theoretical/generic standpoint.
> 
> Then that's a different story. If it will likely involve more specific
> handling, then that function should deal with pfns for which it can DTRT
> and for others warn loudly so that the code gets fixed in time.
> 
> IOW, then it should check for the upper pfn of the direct map here and
> we have two, depending on the page sizes used...

Is something like this close to what you're thinking? I've re-tested with
SNP guests and it seems to work as expected.

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 846e9e53dff0..c09497487c08 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -421,7 +421,12 @@ static int adjust_direct_map(u64 pfn, int rmp_level)
        if (WARN_ON_ONCE(rmp_level > PG_LEVEL_2M))
                return -EINVAL;

-       if (WARN_ON_ONCE(rmp_level == PG_LEVEL_2M && !IS_ALIGNED(pfn, PTRS_PER_PMD)))
+       if (!pfn_valid(pfn))
+               return -EINVAL;
+
+       if (rmp_level == PG_LEVEL_2M &&
+           (!IS_ALIGNED(pfn, PTRS_PER_PMD) ||
+            !pfn_valid(pfn + PTRS_PER_PMD - 1)))
                return -EINVAL;

Note that I removed the WARN_ON_ONCE(), which I think was a bit overzealous
for this check. The one above it for rmp_level > PG_LEVEL_2M I think is more
warranted, since it returns error for a use-case that might in theory become
valid on future hardware, but would result in unecessary 1GB->2MB directmap
splitting if we were to try to implement handling for that before it's
actually needed (which may well be never).

-Mike

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette