Messages in this thread | | | From | "Edgecombe, Rick P" <> | Subject | Re: [PATCH] x86/mm/cpa: Warn if set_memory_XXcrypted() fails | Date | Thu, 26 Oct 2023 01:40:38 +0000 |
| |
On Thu, 2023-10-26 at 00:35 +0000, Michael Kelley (LINUX) wrote: > I think you mean "shared" as indicated by the guest page tables (vs. > "shared" > as the state of the page from the host standpoint). Some precision > on > that distinction seems useful here and in follow-on patches to make > callers' > error handling be correct. As I understand it, the premise is that > if the > guest is accessing a page as private, and the host/VMM has messed > around with the page private/shared status, the confidentiality of > the > VM is protected. The risk of leakage occurs when the guest is > accessing > a page as shared, so kernel code must guard against putting memory > on the free list if the guest page tables are marked shared. >
For TDX, the scenario of concern in the VMM error case is if the page is mapped as shared in the guest page tables *and* it is either also marked as shared in the EPT, or the VMM supports automatically converting it on access. In the attacker scenario, I think the problem is just that it is marked shared in the guest.
I can clarify that it needs to be mapped shared in the guest for there to be a problem, but I don't see how it will help the patches to fix the callers. It seems like too many details for the callers to know about. For example, I think some architectures don't change the PTEs at all. The callers abstract shared and private at a higher level.
> To me, this sentence doesn't fully characterize why panic_on_warn > would be used. You describe one reason, which is a caller that fails > to > properly handle an error and incorrectly puts memory with a "shared" > guest PTE on the free list. But getting an error back also implies > that > something unknown has gone wrong with the CoCo mechanism for > managing private vs. shared pages. Security focused users would not > take the risk of continuing to operate with that kind of unknown > error > in the core mechanism of a CoCo VM.
Hmm, yea I could see that some users may want to take a hard line and terminate if anything looks strange. The counter point is that the VMM is actually returning a legal error here. It may be strange based on the details of when HyperV and QEMU/KVM would return this error, but not architecturally.
> > > +vmm_fail: > > + WARN_ONCE(1, "CPA VMM failure to convert memory (addr=%p, > > numpages=%d) to %s.\n", > > + (void *)addr, numpages, enc ? "private" : > > "shared"); > > I'm not sure about outputting the "addr" value. It could be > useful, but the %p format specifier hashes the value unless the > kernel is booted with "no_hash_pointers". Should %px be used > so the address is output unmodified?
Unfortunately, I don't think we can print the kernel virtual address because those are supposed to be hidden for security reasons. Ideally, I would prefer to print the PFN, but we won't have it here in the case of vmalloc's. I thought it might be useful to still have some address printed for debugging purposes.
> > > + > > + return -EIO; > > } > > > > static int __set_memory_enc_dec(unsigned long addr, int numpages, > > bool enc) > > -- > > 2.34.1 > > My comments notwithstanding, I'm good with this overall change and > the additional level of protection it offers to CoCo VM users.
Thanks.
| |