[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240127160249.GDZbUpKW_cqRzdYn7Z@fat_crate.local>
Date: Sat, 27 Jan 2024 17:02:49 +0100
From: Borislav Petkov <bp@...en8.de>
To: Michael Roth <michael.roth@....com>
Cc: x86@...nel.org, kvm@...r.kernel.org, linux-coco@...ts.linux.dev,
linux-mm@...ck.org, linux-crypto@...r.kernel.org,
linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
jroedel@...e.de, thomas.lendacky@....com, hpa@...or.com,
ardb@...nel.org, pbonzini@...hat.com, seanjc@...gle.com,
vkuznets@...hat.com, jmattson@...gle.com, luto@...nel.org,
dave.hansen@...ux.intel.com, slp@...hat.com, pgonda@...gle.com,
peterz@...radead.org, srinivas.pandruvada@...ux.intel.com,
rientjes@...gle.com, tobin@....com, vbabka@...e.cz,
kirill@...temov.name, ak@...ux.intel.com, tony.luck@...el.com,
sathyanarayanan.kuppuswamy@...ux.intel.com, alpergun@...gle.com,
jarkko@...nel.org, ashish.kalra@....com, nikunj.dadhania@....com,
pankaj.gupta@....com, liam.merwick@...cle.com
Subject: Re: [PATCH v2 11/25] x86/sev: Adjust directmap to avoid inadvertant
RMP faults
On Sat, Jan 27, 2024 at 09:45:06AM -0600, Michael Roth wrote:
> directmap maps all physical memory accessible by kernel, including text
> pages, so those are valid PFNs as far as this function is concerned.
Why don't you have a look at
Documentation/arch/x86/x86_64/mm.rst
to sync up on the nomenclature first?
ffff888000000000 | -119.5 TB | ffffc87fffffffff | 64 TB | direct mapping of all physical memory (page_offset_base)
...
ffffffff80000000 | -2 GB | ffffffff9fffffff | 512 MB | kernel text mapping, mapped to physical address 0
and so on.
> The expectation is that the caller is aware of what PFNs it is passing in,
There are no expectations. Have you written them down somewhere?
> This function only splits mappings in the 0xffff888000000000 directmap
> range.
This function takes any PFN it gets passed in as it is. I don't care
who its users are now or in the future and whether they pay attention
what they pass into - it needs to be properly defined.
Mike, please get on with the program. Use the right naming for the
function and basta.
IOW, this:
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 0a8f9334ec6e..652ee63e87fd 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -394,7 +394,7 @@ EXPORT_SYMBOL_GPL(psmash);
* More specifics on how these checks are carried out can be found in APM
* Volume 2, "RMP and VMPL Access Checks".
*/
-static int adjust_direct_map(u64 pfn, int rmp_level)
+static int split_pfn(u64 pfn, int rmp_level)
{
unsigned long vaddr = (unsigned long)pfn_to_kaddr(pfn);
unsigned int level;
@@ -405,7 +405,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;
/*
@@ -456,7 +461,7 @@ static int rmpupdate(u64 pfn, struct rmp_state *state)
level = RMP_TO_PG_LEVEL(state->pagesize);
- if (adjust_direct_map(pfn, level))
+ if (split_pfn(pfn, level))
return -EFAULT;
do {
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists