On 7/16/21 7:46 PM, Sean Christopherson wrote:
> takes the page size as a parameter even though it unconditionally zeros the page
> size flag in the RMP entry for unassigned pages.
>
> A wrapper around rmpupdate() would definitely help, e.g. (though level might need
> to be an "int" to avoid a bunch of casts).
>
> int rmp_make_shared(u64 pfn, enum pg_level level);
>
> Wrappers for "private" and "firmware" would probably be helpful too. And if you
> do that, I think you can bury both "struct rmpupdate", rmpupdate(), and
> X86_TO_RMP_PG_LEVEL() in arch/x86/kernel/sev.c. snp_set_rmptable_state() might
> need some refactoring to avoid three booleans, but I guess maybe that could be
> an exception? Not sure. Anyways, was thinking something like:
>
> int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, int asid);
> int rmp_make_firmware(u64 pfn);
>
> It would consolidate a bit of code, and more importantly it would give visual
> cues to the reader, e.g. it's easy to overlook "val = {0}" meaning "make shared".
Okay, I will add helper to make things easier. One case where we will
need to directly call the rmpupdate() is during the LAUNCH_UPDATE
command. In that case the page is private and its immutable bit is also
set. This is because the firmware makes change to the page, and we are
required to set the immutable bit before the call.
>
> Side topic, what happens if a firmware entry is configured with page_size=1?
Its not any different from the guest requesting a page private with the
page_size=1. Some firmware commands require the page_size=0, and others
can work with page_size=1 or page_size=0.
>
> And one architectural question: what prevents a malicious VMM from punching a 4k
> shared page into a 2mb private page? E.g.
>
> rmpupdate(1 << 20, [private, 2mb]);
> rmpupdate(1 << 20 + 4096, [shared, 4kb]);
>
> I don't see any checks in the pseudocode that will detect this, and presumably the
> whole point of a 2mb private RMP entry is to not have to go walk the individual
> 4kb entries on a private access.
I believe pseudo-code is not meant to be exactly accurate and
comprehensive, but it is intended to summarize the HW behavior and
explain what can cause the different fault cases. In the real design we
may have a separate checks to catch the above issue. I just tested on
the hardware to ensure that HW correctly detects the above error
condition. However, in this case we are missing a significant check (at
least the check that the 2M region is not already assigned). I have
raised the concern with the hardware team to look into updating the APM.
thank you so much for the bringing this up.
thanks
