From: Edgecombe, Rick P <rick.p.edgecombe@intel.com> Sent: Thursday, August 31, 2023 10:30 AM

On Wed, 2023-08-30 at 16:40 -0700, Rick Edgecombe wrote:

quoted

This is a bit of an existing problem, but the failure cases of these
set_memory_en/decrypted() operations does not look to be in great
shape. It could fail halfway through if it needs to split the direct
map under memory pressure, in which case some of the callers will see
the error and free the unmapped pages to the direct map. (I was
looking at dma_direct_alloc()) Other's just leak the pages.

See further comments below.

quoted

But the situation before the patch is not much better, since the direct
map change or enc_status_change_prepare/finish() could fail and leave
the pages in an inconsistent state, like this patch is trying to address.

This lack of rollback on failure for CPA calls needs particular odd
handling in all the set_memory() callers. The way is to make a CPA call
to restore it to the previous permission, regardless of the error code
returned in the initial call that failed. The callers depend on any PTE
change successfully made having any needed splits already done for
those PTEs, so the restore can succeed at least as far as the failed
CPA call got.

Wait, since this does set_memory_np() as the first step for both
set_memory_encrypted() and set_memory_decrypted(), that pattern in the
callers wouldn't work. I wonder if it should try to rollback itself if
set_memory_np() fails (call set_memory_p() before returning the error).
At least that will handle failures that happen on the guest side.

Yes, I agree the error handling is very limited.  I'll try to make my
patch cleanup properly if set_memory_np() fails as step 1.  In general,
complete error cleanup on private <-> shared transitions looks to be
pretty hard, and the original implementation obviously didn't deal
with it.  For most of the steps in the sequence, a failure indicates
something is pretty seriously broken with the CoCo aspects of the
VM, and it's not clear that trying to clean up is likely to succeed or
will make things any better.  

quoted

In this COCO case apparently the enc_status_change_prepare/finish()
could fail too (and maybe not have the same forward progress
behavior?). So I'm not sure what you can do in that case.

Exactly.

quoted

I'm also not sure how bad it is to free encryption mismatched pages.  Is
it the same as freeing unmapped pages? (likely oops or panic)

It's bad to free mismatched pages.  You're just setting a time bomb
for some other code to encounter later.  It will either cause an
oops/panic, or will allow the VM to unknowingly put data in a page
that is visible to the hypervisor and violate the tenets of being a
CoCo VM.  In the code I've worked with, we don't free any memory
where set_memory_encrypted() or set_memory_decrypted() has failed.
We assume there hasn't been a cleanup and hence the state and
consistency of the memory is unknown.

I think there's a decent argument for not investing too much effort
in the cleanup paths for private <-> shared transitions.  A failure
indicates that something is seriously wrong, from which full recovery
is unlikely.  Callers of set_memory_encrypted()/decrypted() should
assume that a failure leaves the memory in an inconsistent state,
and the memory should just be leaked until the VM can be shutdown.
Some strong comments along these lines could be added to
set_memory_encrypted()/decrypted().

I'm going to be out on vacation until mid-September, so it will be
a couple of weeks before I get back to doing a full patch set that
responds to these and other comments.

Michael