On Wed, Feb 7, 2024 at 3:43 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > 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?
>
> As proposed, snp_launch_update_gfn_handler() doesn't verify the state of the
> gfns' attributes. But that's a minor problem and probably not a sticking point.
>
> My overarching complaint is that the code is to be wildly unsafe, or at the very
> least brittle. Without guest_memfd's knowledge, and without holding any locks
> beyond kvm->lock, it
>
> 1) checks if a pfn is shared in the RMP
> 2) copies data to the page
> 3) converts the page to private in the RMP
> 4) does PSP stuff
> 5) on failure, converts the page back to shared in RMP
> 6) conditionally on failure, writes to the page via a gfn
>
> I'm not at all confident that 1-4 isn't riddled with TOCTOU bugs, and that's
> before KVM gains support for intrahost migration, i.e. before KVM allows multiple
> VM instances to bind to a single guest_memfd.
Absolutely.
> But I _think_ we mostly sorted this out at PUCK. IIRC, the plan is to have guest_memfd
> provide (kernel) APIs to allow arch/vendor code to initialize a guest_memfd range.
> That will give guest_memfd complete control over the state of a given page, will
> allow guest_memfd to take the appropriate locks, and if we're lucky, will be reusable
> by other CoCo flavors beyond SNP.
Ok, either way it's clear that guest_memfd needs to be in charge.
Whether it's MEMORY_ENCRYPT_OP that calls into guest_memfd or vice
versa, that only matters so much.
> > > 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.
>
> This proposed KVM code:
>
> + host_rmp_make_shared(pfns[i], PG_LEVEL_4K, true);
> +
> + ret = kvm_write_guest_page(kvm, gfn, kvaddr, 0, PAGE_SIZE);
> + if (ret)
> + pr_err("Failed to write CPUID page back to userspace, ret: 0x%x\n",
> + ret);
>
>
> I have no objection to propagating error/debug information back to userspace,
> but it needs to be routed through the source page (or I suppose some dedicated
> error page, but that seems like overkill). Shoving the error information into
> guest memory is gross.
Yes, but it should be just a consequence of not actually using
start_gfn. Having to copy back remains weird, but what can you do.
Paolo
