On Fri, Feb 2, 2024 at 11:55 PM Sean Christopherson [off-list ref] wrote:

quoted

quoted

quoted

+        struct kvm_sev_snp_launch_update {
+                __u64 start_gfn;        /* Guest page number to start from. */
+                __u64 uaddr;            /* userspace address need to be encrypted */

Huh?  Why is KVM taking a userspace address?  IIUC, the address unconditionally
gets translated into a gfn, so why not pass a gfn?

And speaking of gfns, AFAICT start_gfn is never used.

I think having both the uaddr and start_gfn parameters makes sense. I
think it's only awkward because how I'm using the memslot to translate
the uaddr to a GFN in the current implementation,

Yes.

quoted

quoted

Oof, reading more of the code, this *requires* an effective in-place copy-and-convert
of guest memory.

So that's how it's done here, KVM_SNP_LAUNCH_UPDATE copies the pages into
gmem, then passes those pages on to firmware for encryption. Then the
VMM is expected to mark the GFN range as private via
KVM_SET_MEMORY_ATTRIBUTES, since the VMM understands what constitutes
initial private/encrypted payload. I should document that better in
KVM_SNP_LAUNCH_UPDATE docs however.

That's fine.  As above, my complaints are that the unencrypted source *must* be
covered by a memslot.  That's beyond ugly.

Yes, if there's one field that has to go it's uaddr, and then you'll
have a non-in-place encrypt (any copy performed by KVM it is hidden).

quoted

quoted

quoted

+         kvaddr = pfn_to_kaddr(pfns[i]);
+         if (!virt_addr_valid(kvaddr)) {

I really, really don't like that this assume guest_memfd is backed by struct page.

There are similar enforcements in the SEV/SEV-ES code. There was some
initial discussion about relaxing this for SNP so we could support
things like /dev/mem-mapped guest memory, but then guest_memfd came
along and made that to be an unlikely use-case in the near-term given
that it relies on alloc_pages() currently and explicitly guards against
mmap()'ing pages in userspace.

I think it makes to keep the current tightened restrictions in-place
until such a use-case comes along, since otherwise we are relaxing a
bunch of currently-useful sanity checks that span all throughout the code

What sanity is being checked for, in other words why are they useful?
If all you get for breaking the promise is a KVM_BUG_ON, for example,
that's par for the course. If instead you get an oops, then we have a
problem.

I may be a bit less draconian than Sean, but the assumptions need to
be documented and explained because they _are_ going to go away.

quoted

quoted

(b) Why are KVM's memory attributes never consulted?

It doesn't really matter if the attributes are set before or after
KVM_SNP_LAUNCH_UPDATE, only that by the time the guest actually launches
they pages get set to private so they get faulted in from gmem. We could
document our expectations and enforce them here if that's preferable
however. Maybe requiring KVM_SET_MEMORY_ATTRIBUTES(private) in advance
would make it easier to enforce that userspace does the right thing.
I'll see how that looks if there are no objections.

Userspace owns whether a page is PRIVATE or SHARED, full stop.  If KVM can't
honor that, then we need to come up with better uAPI.

Can you explain more verbosely what you mean?

quoted

quoted

quoted

+                  * When invalid CPUID function entries are detected, the firmware
+                  * corrects these entries for debugging purpose and leaves the
+                  * page unencrypted so it can be provided users for debugging
+                  * and error-reporting.

Why?  IIUC, this is basically backdooring reads/writes into guest_memfd to avoid
having to add proper mmap() support.

Yes, I am specifically complaining about writing guest memory on failure, which is
all kinds of weird.

It is weird but I am not sure if you are complaining about firmware
behavior or something else.

Paolo