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

[Michael Roth]

On Sat, Jan 27, 2024 at 12:42:07PM +0100, Borislav Petkov wrote:
> On Fri, Jan 26, 2024 at 05:54:20PM -0600, Michael Roth wrote:
> > 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))
> 
> _text at VA 0xffffffff81000000 is also a valid pfn so no, this is not
> enough.

directmap maps all physical memory accessible by kernel, including text
pages, so those are valid PFNs as far as this function is concerned. We
can't generally guard against the caller passing in any random PFN that
might also be mapped into additional address ranges, similarly to how
we can't guard against something doing a write to some random PFN
__va(0x1234) and scribbling over memory that it doesn't own, or just
unmapping the entire directmap range and blowing up the kernel.

The expectation is that the caller is aware of what PFNs it is passing in,
whether those PFNs have additional mappings, and if those mappings are of
concern, implement the necessary handlers if new use-cases are ever
introducted, like the adjust_kernel_text_mapping() example I mentioned
earlier. 

> 
> Either this function should not have "direct map" in the name as it
> converts *any* valid pfn not just the direct map ones or it should check
> whether the pfn belongs to the direct map range.

This function only splits mappings in the 0xffff888000000000 directmap
range. It would be inaccurate to name it in such a way that suggests
that it does anything else. If a use-case ever arises for splitting
_text mappings at 0xffffffff81000000, or any other ranges, those too
would best served by dedicated helpers adjust_kernel_text_mapping()
that *actually* modify mappings for those virtual ranges, and implement
bounds-checking appropriate for those physical/virtual ranges. The
directmap range maps all kernel-accessible physical memory, so it's
appropriate that our bounds-checking for the purpose of
adjust_direct_map() is all kernel-accessible physical memory. If that
includes PFNs mapped to other virtual ranges as well, the caller needs
to consider that and implement additional helpers as necessary, but
they'd likely *still* need to call adjust_direct_map() to adjust the
directmap range in addition to those other ranges, so even if were
possible to do so reliably, we shouldn't try to selectively reject any
PFN ranges beyond what mm.rst suggests is valid for 0xffff888000000000.

-Mike

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