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

Borislav Petkov
2024-01-27T16:10:52Z

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.