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

[Michael Roth]

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