| Message ID | 20240222161047.402609-5-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Restricted mapping of guest_memfd at the host and pKVM/arm64 support | expand |
On Thu, Feb 22, 2024, Fuad Tabba wrote: > +#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM_MAPPABLE > +bool kvm_is_gmem_mapped(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) > +{ > + struct kvm_memslot_iter iter; > + > + kvm_for_each_memslot_in_gfn_range(&iter, kvm_memslots(kvm), gfn_start, gfn_end) { > + struct kvm_memory_slot *memslot = iter.slot; > + gfn_t start, end, i; > + > + start = max(gfn_start, memslot->base_gfn); > + end = min(gfn_end, memslot->base_gfn + memslot->npages); > + if (WARN_ON_ONCE(start >= end)) > + continue; > + > + for (i = start; i < end; i++) { > + struct page *page; > + bool is_mapped; > + kvm_pfn_t pfn; > + int ret; > + > + /* > + * Check the page_mapcount with the page lock held to > + * avoid racing with kvm_gmem_fault(). > + */ I don't see how this avoids a TOCTOU race. kvm_gmem_fault() presumably runs with mmap_lock, but it definitely doesn't take slots_lock. And this has slots_lock, but definitely doesn't have mmap_lock. If the fault is blocked on the page lock, this will see page_mapcount() = 0, and the fault will map the page as soon as unlock_page() runs. Am I missing something? I haven't thought deeply about this, but I'm pretty sure that "can this be mapped" needs to tracked against the guest_memfd() inode, not in KVM. While each guest_memfd() *file* has a 1:1 binding with a KVM instance, the plan is to allow multiple files per inode, e.g. to allow intra-host migration to a new KVM instance, without destroying guest_memfd().
On 22.02.24 17:10, Fuad Tabba wrote: > Guest private memory should never be mapped by the host. > Therefore, do not allow setting the private attribute to guest > memory if that memory is mapped by the host. > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > include/linux/kvm_host.h | 7 ++++++ > virt/kvm/kvm_main.c | 51 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index fad296baa84e..f52d5503ddef 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -2408,11 +2408,18 @@ static inline bool kvm_gmem_is_mappable(struct kvm *kvm, gfn_t gfn) > return !(kvm_get_memory_attributes(kvm, gfn) & > KVM_MEMORY_ATTRIBUTE_NOT_MAPPABLE); > } > + > +bool kvm_is_gmem_mapped(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end); > #else > static inline bool kvm_gmem_is_mappable(struct kvm *kvm, gfn_t gfn) > { > return false; > } > + > +static inline bool kvm_is_gmem_mapped(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) > +{ > + return false; > +} > #endif /* CONFIG_KVM_GENERIC_PRIVATE_MEM_MAPPABLE */ > > #endif > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index fba4dc6e4107..9f6ff314bda3 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2516,6 +2516,48 @@ static __always_inline void kvm_handle_gfn_range(struct kvm *kvm, > KVM_MMU_UNLOCK(kvm); > } > > +#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM_MAPPABLE > +bool kvm_is_gmem_mapped(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) > +{ > + struct kvm_memslot_iter iter; > + > + kvm_for_each_memslot_in_gfn_range(&iter, kvm_memslots(kvm), gfn_start, gfn_end) { > + struct kvm_memory_slot *memslot = iter.slot; > + gfn_t start, end, i; > + > + start = max(gfn_start, memslot->base_gfn); > + end = min(gfn_end, memslot->base_gfn + memslot->npages); > + if (WARN_ON_ONCE(start >= end)) > + continue; > + > + for (i = start; i < end; i++) { > + struct page *page; > + bool is_mapped; > + kvm_pfn_t pfn; > + int ret; > + > + /* > + * Check the page_mapcount with the page lock held to > + * avoid racing with kvm_gmem_fault(). > + */ > + ret = kvm_gmem_get_pfn_locked(kvm, memslot, i, &pfn, NULL); > + if (WARN_ON_ONCE(ret)) > + continue; > + > + page = pfn_to_page(pfn); > + is_mapped = page_mapcount(page); > + unlock_page(page); > + put_page(page); Stumbling over this, please avoid using page_mapcount(). page_mapped() -- or better folio_mapped() -- is what you should be using here (iow, convert it to work on folios).
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index fad296baa84e..f52d5503ddef 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -2408,11 +2408,18 @@ static inline bool kvm_gmem_is_mappable(struct kvm *kvm, gfn_t gfn) return !(kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_NOT_MAPPABLE); } + +bool kvm_is_gmem_mapped(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end); #else static inline bool kvm_gmem_is_mappable(struct kvm *kvm, gfn_t gfn) { return false; } + +static inline bool kvm_is_gmem_mapped(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) +{ + return false; +} #endif /* CONFIG_KVM_GENERIC_PRIVATE_MEM_MAPPABLE */ #endif diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index fba4dc6e4107..9f6ff314bda3 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2516,6 +2516,48 @@ static __always_inline void kvm_handle_gfn_range(struct kvm *kvm, KVM_MMU_UNLOCK(kvm); } +#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM_MAPPABLE +bool kvm_is_gmem_mapped(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) +{ + struct kvm_memslot_iter iter; + + kvm_for_each_memslot_in_gfn_range(&iter, kvm_memslots(kvm), gfn_start, gfn_end) { + struct kvm_memory_slot *memslot = iter.slot; + gfn_t start, end, i; + + start = max(gfn_start, memslot->base_gfn); + end = min(gfn_end, memslot->base_gfn + memslot->npages); + if (WARN_ON_ONCE(start >= end)) + continue; + + for (i = start; i < end; i++) { + struct page *page; + bool is_mapped; + kvm_pfn_t pfn; + int ret; + + /* + * Check the page_mapcount with the page lock held to + * avoid racing with kvm_gmem_fault(). + */ + ret = kvm_gmem_get_pfn_locked(kvm, memslot, i, &pfn, NULL); + if (WARN_ON_ONCE(ret)) + continue; + + page = pfn_to_page(pfn); + is_mapped = page_mapcount(page); + unlock_page(page); + put_page(page); + + if (is_mapped) + return true; + } + } + + return false; +} +#endif /* CONFIG_KVM_GENERIC_PRIVATE_MEM_MAPPABLE */ + static bool kvm_pre_set_memory_attributes(struct kvm *kvm, struct kvm_gfn_range *range) { @@ -2565,6 +2607,15 @@ static int __kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end, if (kvm_range_has_memory_attributes(kvm, start, end, attributes)) goto out_unlock; + if (IS_ENABLED(CONFIG_KVM_GENERIC_PRIVATE_MEM_MAPPABLE) && userspace) { + /* Host-mapped memory cannot be private. */ + if ((attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE) && + kvm_is_gmem_mapped(kvm, start, end)) { + r = -EPERM; + goto out_unlock; + } + } + /* * Reserve memory ahead of time to avoid having to deal with failures * partway through setting the new attributes.
Guest private memory should never be mapped by the host. Therefore, do not allow setting the private attribute to guest memory if that memory is mapped by the host. Signed-off-by: Fuad Tabba <tabba@google.com> --- include/linux/kvm_host.h | 7 ++++++ virt/kvm/kvm_main.c | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+)