diff mbox series

[RFCv2,15/16] KVM: Unmap protected pages from direct mapping

Message ID 20201020061859.18385-16-kirill.shutemov@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series KVM protected memory extension | expand

Commit Message

Kirill A. Shutemov Oct. 20, 2020, 6:18 a.m. UTC
If the protected memory feature enabled, unmap guest memory from
kernel's direct mappings.

Migration and KSM is disabled for protected memory as it would require a
special treatment.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/mm.h       |  3 +++
 mm/huge_memory.c         |  8 ++++++++
 mm/ksm.c                 |  2 ++
 mm/memory.c              | 12 ++++++++++++
 mm/rmap.c                |  4 ++++
 virt/lib/mem_protected.c | 21 +++++++++++++++++++++
 6 files changed, 50 insertions(+)

Comments

Peter Zijlstra Oct. 20, 2020, 7:12 a.m. UTC | #1
On Tue, Oct 20, 2020 at 09:18:58AM +0300, Kirill A. Shutemov wrote:
> If the protected memory feature enabled, unmap guest memory from
> kernel's direct mappings.
> 
> Migration and KSM is disabled for protected memory as it would require a
> special treatment.

How isn't disabling migration a problem? Are we going to back allocation
by CMA instead?
David Hildenbrand Oct. 20, 2020, 12:18 p.m. UTC | #2
On 20.10.20 08:18, Kirill A. Shutemov wrote:
> If the protected memory feature enabled, unmap guest memory from
> kernel's direct mappings.

Gah, ugly. I guess this also defeats compaction, swapping, ... oh gosh.
As if all of the encrypted VM implementations didn't bring us enough
ugliness already (SEV extensions also don't support reboots, but can at
least kexec() IIRC).

Something similar is done with secretmem [1]. And people don't seem to
like fragmenting the direct mapping (including me).

[1] https://lkml.kernel.org/r/20200924132904.1391-1-rppt@kernel.org
David Hildenbrand Oct. 20, 2020, 1:20 p.m. UTC | #3
On 20.10.20 14:18, David Hildenbrand wrote:
> On 20.10.20 08:18, Kirill A. Shutemov wrote:
>> If the protected memory feature enabled, unmap guest memory from
>> kernel's direct mappings.
> 
> Gah, ugly. I guess this also defeats compaction, swapping, ... oh gosh.
> As if all of the encrypted VM implementations didn't bring us enough
> ugliness already (SEV extensions also don't support reboots, but can at
> least kexec() IIRC).
> 
> Something similar is done with secretmem [1]. And people don't seem to
> like fragmenting the direct mapping (including me).
> 
> [1] https://lkml.kernel.org/r/20200924132904.1391-1-rppt@kernel.org
> 

I just thought "hey, we might have to replace pud/pmd mappings by page
tables when calling kernel_map_pages", this can fail with -ENOMEM, why
isn't there proper error handling.

Then I dived into __kernel_map_pages() which states:

"The return value is ignored as the calls cannot fail. Large pages for
identity mappings are not used at boot time and hence no memory
allocations during large page split."

I am probably missing something important, but how is calling
kernel_map_pages() safe *after* booting?! I know we use it for
debug_pagealloc(), but using it in a production-ready feature feels
completely irresponsible. What am I missing?
Edgecombe, Rick P Oct. 21, 2020, 1:20 a.m. UTC | #4
On Tue, 2020-10-20 at 15:20 +0200, David Hildenbrand wrote:
> On 20.10.20 14:18, David Hildenbrand wrote:
> > On 20.10.20 08:18, Kirill A. Shutemov wrote:
> > > If the protected memory feature enabled, unmap guest memory from
> > > kernel's direct mappings.
> > 
> > Gah, ugly. I guess this also defeats compaction, swapping, ... oh
> > gosh.
> > As if all of the encrypted VM implementations didn't bring us
> > enough
> > ugliness already (SEV extensions also don't support reboots, but
> > can at
> > least kexec() IIRC).
> > 
> > Something similar is done with secretmem [1]. And people don't seem
> > to
> > like fragmenting the direct mapping (including me).
> > 
> > [1] https://lkml.kernel.org/r/20200924132904.1391-1-rppt@kernel.org
> > 
> 
> I just thought "hey, we might have to replace pud/pmd mappings by
> page
> tables when calling kernel_map_pages", this can fail with -ENOMEM,
> why
> isn't there proper error handling.
> 
> Then I dived into __kernel_map_pages() which states:
> 
> "The return value is ignored as the calls cannot fail. Large pages
> for
> identity mappings are not used at boot time and hence no memory
> allocations during large page split."
> 
> I am probably missing something important, but how is calling
> kernel_map_pages() safe *after* booting?! I know we use it for
> debug_pagealloc(), but using it in a production-ready feature feels
> completely irresponsible. What am I missing?
> 

My understanding was that DEBUG_PAGEALLOC maps the direct map at 4k
post-boot as well so that kernel_map_pages() can't fail for it, and to
avoid having to take locks for splits in interrupts. I think you are on
to something though. That function is essentially a set_memory_()
functions on x86 at least, and many callers do not check the error
codes. Kees Cook has been pushing to fix this actually: 
https://github.com/KSPP/linux/issues/7

As long as a page has been broken to 4k, it should be able to be re-
mapped without failure (like debug page alloc relies on). If an initial
call to restrict permissions needs and fails to break a page, its remap
does not need to succeed either before freeing the page, since the page
never got set with a lower permission. That's my understanding from
x86's cpa at least. The problem becomes that the page silently doesn't
have its expected protections during use. Unchecked set_memory_()
caching property change failures might result in functional problems
though.

So there is some background if you wanted it, but yea, I agree this
feature should handle if the unmap failed. Probably return an error on
the IOCTL and maybe the hypercall. kernel_map_pages() also doesn't do a
shootdown though, so this direct map protection is more in the best
effort category in its current state.


For unmapping causing direct map fragmentation. The two techniques
floating around, besides breaking indiscriminately, seem to be:
1. Group and cache unmapped pages to minimize the damage (like secret
mem)
2. Somehow repair them back to large pages when reset RW (as Kirill had
tried earlier this year in another series)

I had imagined this usage would want something like that eventually,
but neither helps with the other limitations you mentioned (migration,
etc).
Edgecombe, Rick P Oct. 21, 2020, 6:49 p.m. UTC | #5
On Tue, 2020-10-20 at 09:18 +0300, Kirill A. Shutemov wrote:
> If the protected memory feature enabled, unmap guest memory from
> kernel's direct mappings.
> 
> Migration and KSM is disabled for protected memory as it would
> require a
> special treatment.
> 
So do we care about this scenario where a malicious userspace causes a
kernel oops? I'm not sure if it's prevented somehow.

CPU0 (exercising other kernel functionality)	CPU1
						mark page shared
page = get_user_pages(!FOLL_KVM)
						mark page private
kmap(page)
access unmapped page and oops
Mike Rapoport Oct. 23, 2020, 12:37 p.m. UTC | #6
On Tue, Oct 20, 2020 at 09:18:58AM +0300, Kirill A. Shutemov wrote:
> If the protected memory feature enabled, unmap guest memory from
> kernel's direct mappings.
> 
> Migration and KSM is disabled for protected memory as it would require a
> special treatment.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  include/linux/mm.h       |  3 +++
>  mm/huge_memory.c         |  8 ++++++++
>  mm/ksm.c                 |  2 ++
>  mm/memory.c              | 12 ++++++++++++
>  mm/rmap.c                |  4 ++++
>  virt/lib/mem_protected.c | 21 +++++++++++++++++++++
>  6 files changed, 50 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index ee274d27e764..74efc51e63f0 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -671,6 +671,9 @@ static inline bool vma_is_kvm_protected(struct vm_area_struct *vma)
>  	return vma->vm_flags & VM_KVM_PROTECTED;
>  }
>  
> +void kvm_map_page(struct page *page, int nr_pages);
> +void kvm_unmap_page(struct page *page, int nr_pages);

This still does not seem right ;-)

And I still think that map/unmap primitives shoud be a part of the
generic mm rather than exported by KVM.

> +
>  #ifdef CONFIG_SHMEM
>  /*
>   * The vma_is_shmem is not inline because it is used only by slow
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ec8cf9a40cfd..40974656cb43 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -627,6 +627,10 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
>  		spin_unlock(vmf->ptl);
>  		count_vm_event(THP_FAULT_ALLOC);
>  		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
> +
> +		/* Unmap page from direct mapping */
> +		if (vma_is_kvm_protected(vma))
> +			kvm_unmap_page(page, HPAGE_PMD_NR);
>  	}
>  
>  	return 0;
> @@ -1689,6 +1693,10 @@ int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  			page_remove_rmap(page, true);
>  			VM_BUG_ON_PAGE(page_mapcount(page) < 0, page);
>  			VM_BUG_ON_PAGE(!PageHead(page), page);
> +
> +			/* Map the page back to the direct mapping */
> +			if (vma_is_kvm_protected(vma))
> +				kvm_map_page(page, HPAGE_PMD_NR);
>  		} else if (thp_migration_supported()) {
>  			swp_entry_t entry;
>  
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 9afccc36dbd2..c720e271448f 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -528,6 +528,8 @@ static struct vm_area_struct *find_mergeable_vma(struct mm_struct *mm,
>  		return NULL;
>  	if (!(vma->vm_flags & VM_MERGEABLE) || !vma->anon_vma)
>  		return NULL;
> +	if (vma_is_kvm_protected(vma))
> +		return NULL;
>  	return vma;
>  }
>  
> diff --git a/mm/memory.c b/mm/memory.c
> index 2c9756b4e52f..e28bd5f902a7 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1245,6 +1245,11 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>  				    likely(!(vma->vm_flags & VM_SEQ_READ)))
>  					mark_page_accessed(page);
>  			}
> +
> +			/* Map the page back to the direct mapping */
> +			if (vma_is_anonymous(vma) && vma_is_kvm_protected(vma))
> +				kvm_map_page(page, 1);
> +
>  			rss[mm_counter(page)]--;
>  			page_remove_rmap(page, false);
>  			if (unlikely(page_mapcount(page) < 0))
> @@ -3466,6 +3471,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  	struct page *page;
>  	vm_fault_t ret = 0;
>  	pte_t entry;
> +	bool set = false;
>  
>  	/* File mapping without ->vm_ops ? */
>  	if (vma->vm_flags & VM_SHARED)
> @@ -3554,6 +3560,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  	inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
>  	page_add_new_anon_rmap(page, vma, vmf->address, false);
>  	lru_cache_add_inactive_or_unevictable(page, vma);
> +	set = true;
>  setpte:
>  	set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
>  
> @@ -3561,6 +3568,11 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  	update_mmu_cache(vma, vmf->address, vmf->pte);
>  unlock:
>  	pte_unmap_unlock(vmf->pte, vmf->ptl);
> +
> +	/* Unmap page from direct mapping */
> +	if (vma_is_kvm_protected(vma) && set)
> +		kvm_unmap_page(page, 1);
> +
>  	return ret;
>  release:
>  	put_page(page);
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9425260774a1..247548d6d24b 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1725,6 +1725,10 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  
>  static bool invalid_migration_vma(struct vm_area_struct *vma, void *arg)
>  {
> +	/* TODO */
> +	if (vma_is_kvm_protected(vma))
> +		return true;
> +
>  	return vma_is_temporary_stack(vma);
>  }
>  
> diff --git a/virt/lib/mem_protected.c b/virt/lib/mem_protected.c
> index 1dfe82534242..9d2ef99285e5 100644
> --- a/virt/lib/mem_protected.c
> +++ b/virt/lib/mem_protected.c
> @@ -30,6 +30,27 @@ void kvm_unmap_page_atomic(void *vaddr)
>  }
>  EXPORT_SYMBOL_GPL(kvm_unmap_page_atomic);
>  
> +void kvm_map_page(struct page *page, int nr_pages)
> +{
> +	int i;
> +
> +	/* Clear page before returning it to the direct mapping */
> +	for (i = 0; i < nr_pages; i++) {
> +		void *p = kvm_map_page_atomic(page + i);
> +		memset(p, 0, PAGE_SIZE);
> +		kvm_unmap_page_atomic(p);
> +	}
> +
> +	kernel_map_pages(page, nr_pages, 1);
> +}
> +EXPORT_SYMBOL_GPL(kvm_map_page);
> +
> +void kvm_unmap_page(struct page *page, int nr_pages)
> +{
> +	kernel_map_pages(page, nr_pages, 0);
> +}
> +EXPORT_SYMBOL_GPL(kvm_unmap_page);
> +
>  int kvm_init_protected_memory(void)
>  {
>  	guest_map_ptes = kmalloc_array(num_possible_cpus(),
> -- 
> 2.26.2
> 
>
Sean Christopherson Oct. 23, 2020, 4:32 p.m. UTC | #7
On Fri, Oct 23, 2020 at 03:37:12PM +0300, Mike Rapoport wrote:
> On Tue, Oct 20, 2020 at 09:18:58AM +0300, Kirill A. Shutemov wrote:
> > If the protected memory feature enabled, unmap guest memory from
> > kernel's direct mappings.
> > 
> > Migration and KSM is disabled for protected memory as it would require a
> > special treatment.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  include/linux/mm.h       |  3 +++
> >  mm/huge_memory.c         |  8 ++++++++
> >  mm/ksm.c                 |  2 ++
> >  mm/memory.c              | 12 ++++++++++++
> >  mm/rmap.c                |  4 ++++
> >  virt/lib/mem_protected.c | 21 +++++++++++++++++++++
> >  6 files changed, 50 insertions(+)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index ee274d27e764..74efc51e63f0 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -671,6 +671,9 @@ static inline bool vma_is_kvm_protected(struct vm_area_struct *vma)
> >  	return vma->vm_flags & VM_KVM_PROTECTED;
> >  }
> >  
> > +void kvm_map_page(struct page *page, int nr_pages);
> > +void kvm_unmap_page(struct page *page, int nr_pages);
> 
> This still does not seem right ;-)
> 
> And I still think that map/unmap primitives shoud be a part of the
> generic mm rather than exported by KVM.

Ya, and going a step further, I suspect it will be cleaner in the long run if
the kernel does not automatically map or unmap when converting between private
and shared/public memory.  Conversions will be rare in a well behaved guest, so
exiting to userspace and forcing userspace to do the unmap->map would not be a
performance bottleneck.  In theory, userspace could also maintain separate
pools for private vs. public mappings, though I doubt any VMM will do that in
practice.
Tom Lendacky Oct. 26, 2020, 7:55 p.m. UTC | #8
On 10/20/20 7:18 AM, David Hildenbrand wrote:
> On 20.10.20 08:18, Kirill A. Shutemov wrote:
>> If the protected memory feature enabled, unmap guest memory from
>> kernel's direct mappings.
> 
> Gah, ugly. I guess this also defeats compaction, swapping, ... oh gosh.
> As if all of the encrypted VM implementations didn't bring us enough
> ugliness already (SEV extensions also don't support reboots, but can at
> least kexec() IIRC).

SEV does support reboot. SEV-ES using Qemu doesn't support reboot because
of the way Qemu resets the vCPU state. If Qemu could relaunch the guest
through the SEV APIs to reset the vCPU state, then a "reboot" would be
possible.

SEV does support kexec, SEV-ES does not at the moment.

Thanks,
Tom

> 
> Something similar is done with secretmem [1]. And people don't seem to
> like fragmenting the direct mapping (including me).
> 
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F20200924132904.1391-1-rppt%40kernel.org&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cb98a5033da37432131b508d874f25194%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637387931403890525%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=BzC%2FeIyOau7BORuUY%2BaiRzYZ%2BOAHANvBDcmV9hpkrts%3D&amp;reserved=0
>
diff mbox series

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ee274d27e764..74efc51e63f0 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -671,6 +671,9 @@  static inline bool vma_is_kvm_protected(struct vm_area_struct *vma)
 	return vma->vm_flags & VM_KVM_PROTECTED;
 }
 
+void kvm_map_page(struct page *page, int nr_pages);
+void kvm_unmap_page(struct page *page, int nr_pages);
+
 #ifdef CONFIG_SHMEM
 /*
  * The vma_is_shmem is not inline because it is used only by slow
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index ec8cf9a40cfd..40974656cb43 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -627,6 +627,10 @@  static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 		spin_unlock(vmf->ptl);
 		count_vm_event(THP_FAULT_ALLOC);
 		count_memcg_event_mm(vma->vm_mm, THP_FAULT_ALLOC);
+
+		/* Unmap page from direct mapping */
+		if (vma_is_kvm_protected(vma))
+			kvm_unmap_page(page, HPAGE_PMD_NR);
 	}
 
 	return 0;
@@ -1689,6 +1693,10 @@  int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 			page_remove_rmap(page, true);
 			VM_BUG_ON_PAGE(page_mapcount(page) < 0, page);
 			VM_BUG_ON_PAGE(!PageHead(page), page);
+
+			/* Map the page back to the direct mapping */
+			if (vma_is_kvm_protected(vma))
+				kvm_map_page(page, HPAGE_PMD_NR);
 		} else if (thp_migration_supported()) {
 			swp_entry_t entry;
 
diff --git a/mm/ksm.c b/mm/ksm.c
index 9afccc36dbd2..c720e271448f 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -528,6 +528,8 @@  static struct vm_area_struct *find_mergeable_vma(struct mm_struct *mm,
 		return NULL;
 	if (!(vma->vm_flags & VM_MERGEABLE) || !vma->anon_vma)
 		return NULL;
+	if (vma_is_kvm_protected(vma))
+		return NULL;
 	return vma;
 }
 
diff --git a/mm/memory.c b/mm/memory.c
index 2c9756b4e52f..e28bd5f902a7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1245,6 +1245,11 @@  static unsigned long zap_pte_range(struct mmu_gather *tlb,
 				    likely(!(vma->vm_flags & VM_SEQ_READ)))
 					mark_page_accessed(page);
 			}
+
+			/* Map the page back to the direct mapping */
+			if (vma_is_anonymous(vma) && vma_is_kvm_protected(vma))
+				kvm_map_page(page, 1);
+
 			rss[mm_counter(page)]--;
 			page_remove_rmap(page, false);
 			if (unlikely(page_mapcount(page) < 0))
@@ -3466,6 +3471,7 @@  static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 	struct page *page;
 	vm_fault_t ret = 0;
 	pte_t entry;
+	bool set = false;
 
 	/* File mapping without ->vm_ops ? */
 	if (vma->vm_flags & VM_SHARED)
@@ -3554,6 +3560,7 @@  static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 	inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
 	page_add_new_anon_rmap(page, vma, vmf->address, false);
 	lru_cache_add_inactive_or_unevictable(page, vma);
+	set = true;
 setpte:
 	set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
 
@@ -3561,6 +3568,11 @@  static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 	update_mmu_cache(vma, vmf->address, vmf->pte);
 unlock:
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
+
+	/* Unmap page from direct mapping */
+	if (vma_is_kvm_protected(vma) && set)
+		kvm_unmap_page(page, 1);
+
 	return ret;
 release:
 	put_page(page);
diff --git a/mm/rmap.c b/mm/rmap.c
index 9425260774a1..247548d6d24b 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1725,6 +1725,10 @@  static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 
 static bool invalid_migration_vma(struct vm_area_struct *vma, void *arg)
 {
+	/* TODO */
+	if (vma_is_kvm_protected(vma))
+		return true;
+
 	return vma_is_temporary_stack(vma);
 }
 
diff --git a/virt/lib/mem_protected.c b/virt/lib/mem_protected.c
index 1dfe82534242..9d2ef99285e5 100644
--- a/virt/lib/mem_protected.c
+++ b/virt/lib/mem_protected.c
@@ -30,6 +30,27 @@  void kvm_unmap_page_atomic(void *vaddr)
 }
 EXPORT_SYMBOL_GPL(kvm_unmap_page_atomic);
 
+void kvm_map_page(struct page *page, int nr_pages)
+{
+	int i;
+
+	/* Clear page before returning it to the direct mapping */
+	for (i = 0; i < nr_pages; i++) {
+		void *p = kvm_map_page_atomic(page + i);
+		memset(p, 0, PAGE_SIZE);
+		kvm_unmap_page_atomic(p);
+	}
+
+	kernel_map_pages(page, nr_pages, 1);
+}
+EXPORT_SYMBOL_GPL(kvm_map_page);
+
+void kvm_unmap_page(struct page *page, int nr_pages)
+{
+	kernel_map_pages(page, nr_pages, 0);
+}
+EXPORT_SYMBOL_GPL(kvm_unmap_page);
+
 int kvm_init_protected_memory(void)
 {
 	guest_map_ptes = kmalloc_array(num_possible_cpus(),