On 6/26/24 14:54, Sean Christopherson wrote: > 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. Not quite. It depends on the page state associated with the page. For example, Hypervisor-Fixed pages have the immutable bit set, but can be read and written. The page states are documented in the SNP API (Chapter 5, Page Management): https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf Thanks, Tom > >> 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.
