linux-kernel - Re: [PATCH v2 11/25] x86/sev: Adjust directmap to avoid inadvertant RMP faults

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]

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