On Wed, Jun 26, 2024, Michael Roth wrote: > On Wed, Jun 26, 2024 at 10:13:44AM -0700, Sean Christopherson wrote: > > On Wed, Jun 26, 2024, Michael Roth wrote: > > > On Wed, Jun 26, 2024 at 06:58:09AM -0700, Sean Christopherson wrote: > > > > [*] https://lore.kernel.org/all/20240229025759.1187910-1-stevensd@xxxxxxxxxx > > > > > > > > > + if (is_error_noslot_pfn(req_pfn)) > > > > > + return -EINVAL; > > > > > + > > > > > + resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa)); > > > > > + if (is_error_noslot_pfn(resp_pfn)) { > > > > > + ret = EINVAL; > > > > > + goto release_req; > > > > > + } > > > > > + > > > > > + if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) { > > > > > + ret = -EINVAL; > > > > > + kvm_release_pfn_clean(resp_pfn); > > > > > + goto release_req; > > > > > + } > > > > > > > > I don't see how this is safe. KVM holds no locks, i.e. can't guarantee that the > > > > resp_pfn stays private for the duration of the operation. And on the opposite > > > > > > When the page is set to private with asid=0,immutable=true arguments, > > > this puts the page in a special 'firmware-owned' state that specifically > > > to avoid any changes to the page state happening from under the ASPs feet. > > > The only way to switch the page to any other state at this point is to > > > issue the SEV_CMD_SNP_PAGE_RECLAIM request to the ASP via > > > snp_page_reclaim(). > > > > > > I could see the guest shooting itself in the foot by issuing 2 guest > > > requests with the same req_pfn/resp_pfn, but on the KVM side whichever > > > request issues rmp_make_private() first would succeed, and then the > > > 2nd request would generate an EINVAL to userspace. > > > > > > In that sense, rmp_make_private()/snp_page_reclaim() sort of pair to > > > lock/unlock a page that's being handed to the ASP. But this should be > > > better documented either way. > > > > What about the host kernel though? I don't see anything here that ensures resp_pfn > > isn't "regular" memory, i.e. that ensure the page isn't being concurrently accessed > > by the host kernel (or some other userspace process). > > > > Or is the "private" memory still accessible by the host? > > It's accessible, but it is immutable according to RMP table, so so it would > require KVM to be elsewhere doing a write to the page, I take it "immutable" means "read-only"? If so, it would be super helpful to document that in the APM. I assumed "immutable" only meant that the RMP entry itself is immutable, and that Assigned=AMD-SP is what prevented host accesses. > but that seems possible if the guest is misbehaved. So I do think the RMP #PF > concerns are warranted, and that looking at using KVM-allocated > intermediary/"bounce" pages to pass to firmware is definitely worth looking > into for v2 as that's just about the safest way to guarantee nothing else > will be writing to the page after it gets set to immutable/firmware-owned.
