diff mbox

[PATCHv5,02/19] mm: Do not use zero page in encrypted pages

Message ID 20180717112029.42378-3-kirill.shutemov@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kirill A. Shutemov July 17, 2018, 11:20 a.m. UTC
Zero page is not encrypted and putting it into encrypted VMA produces
garbage.

We can map zero page with KeyID-0 into an encrypted VMA, but this would
be violation security boundary between encryption domains.

Forbid zero pages in encrypted VMAs.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/s390/include/asm/pgtable.h | 2 +-
 include/linux/mm.h              | 4 ++--
 mm/huge_memory.c                | 3 +--
 mm/memory.c                     | 3 +--
 4 files changed, 5 insertions(+), 7 deletions(-)

Comments

Dave Hansen July 18, 2018, 5:36 p.m. UTC | #1
On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> Zero page is not encrypted and putting it into encrypted VMA produces
> garbage.
> 
> We can map zero page with KeyID-0 into an encrypted VMA, but this would
> be violation security boundary between encryption domains.

Why?  How is it a violation?

It only matters if they write secrets.  They can't write secrets to the
zero page.

Is this only because you accidentally inherited ->vm_page_prot on the
zero page PTE?
Kirill A. Shutemov July 19, 2018, 7:16 a.m. UTC | #2
On Wed, Jul 18, 2018 at 10:36:24AM -0700, Dave Hansen wrote:
> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> > Zero page is not encrypted and putting it into encrypted VMA produces
> > garbage.
> > 
> > We can map zero page with KeyID-0 into an encrypted VMA, but this would
> > be violation security boundary between encryption domains.
> 
> Why?  How is it a violation?
> 
> It only matters if they write secrets.  They can't write secrets to the
> zero page.

I believe usage of zero page is wrong here. It would indirectly reveal
content of supposedly encrypted memory region.

I can see argument why it should be okay and I don't have very strong
opinion on this.

If folks see it's okay to use zero page in encrypted VMAs I can certainly
make it work.

> Is this only because you accidentally inherited ->vm_page_prot on the
> zero page PTE?

Yes, in previous patchset I mapped zero page with wrong KeyID. This is one
of possible fixes for this.
Dave Hansen July 19, 2018, 1:58 p.m. UTC | #3
On 07/19/2018 12:16 AM, Kirill A. Shutemov wrote:
> On Wed, Jul 18, 2018 at 10:36:24AM -0700, Dave Hansen wrote:
>> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
>>> Zero page is not encrypted and putting it into encrypted VMA produces
>>> garbage.
>>>
>>> We can map zero page with KeyID-0 into an encrypted VMA, but this would
>>> be violation security boundary between encryption domains.
>> Why?  How is it a violation?
>>
>> It only matters if they write secrets.  They can't write secrets to the
>> zero page.
> I believe usage of zero page is wrong here. It would indirectly reveal
> content of supposedly encrypted memory region.
> 
> I can see argument why it should be okay and I don't have very strong
> opinion on this.

I think we should make the zero page work.  If folks are
security-sensitive, they need to write to guarantee it isn't being
shared.  That's a pretty low bar.

I'm struggling to think of a case where an attacker has access to the
encrypted data, the virt->phys mapping, *and* can glean something
valuable from the presence of the zero page.

Please spend some time and focus on your patch descriptions.  Use facts
that are backed up and are *precise* or tell the story of how your patch
was developed.  In this case, citing the "security boundary" is not
precise enough without explaining what the boundary is and how it is
violated.
Kirill A. Shutemov July 20, 2018, 12:16 p.m. UTC | #4
On Thu, Jul 19, 2018 at 06:58:14AM -0700, Dave Hansen wrote:
> On 07/19/2018 12:16 AM, Kirill A. Shutemov wrote:
> > On Wed, Jul 18, 2018 at 10:36:24AM -0700, Dave Hansen wrote:
> >> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> >>> Zero page is not encrypted and putting it into encrypted VMA produces
> >>> garbage.
> >>>
> >>> We can map zero page with KeyID-0 into an encrypted VMA, but this would
> >>> be violation security boundary between encryption domains.
> >> Why?  How is it a violation?
> >>
> >> It only matters if they write secrets.  They can't write secrets to the
> >> zero page.
> > I believe usage of zero page is wrong here. It would indirectly reveal
> > content of supposedly encrypted memory region.
> > 
> > I can see argument why it should be okay and I don't have very strong
> > opinion on this.
> 
> I think we should make the zero page work.  If folks are
> security-sensitive, they need to write to guarantee it isn't being
> shared.  That's a pretty low bar.
> 
> I'm struggling to think of a case where an attacker has access to the
> encrypted data, the virt->phys mapping, *and* can glean something
> valuable from the presence of the zero page.

Okay.

> Please spend some time and focus on your patch descriptions.  Use facts
> that are backed up and are *precise* or tell the story of how your patch
> was developed.  In this case, citing the "security boundary" is not
> precise enough without explaining what the boundary is and how it is
> violated.

Fair enough. I'll go though all commit messages once again. Sorry.
diff mbox

Patch

diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 5ab636089c60..2e8658962aae 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -505,7 +505,7 @@  static inline int mm_alloc_pgste(struct mm_struct *mm)
  * In the case that a guest uses storage keys
  * faults should no longer be backed by zero pages
  */
-#define mm_forbids_zeropage mm_has_pgste
+#define vma_forbids_zeropage(vma) mm_has_pgste(vma->vm_mm)
 static inline int mm_uses_skeys(struct mm_struct *mm)
 {
 #ifdef CONFIG_PGSTE
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c8780c5835ad..151d6e6b16e5 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -92,8 +92,8 @@  extern int mmap_rnd_compat_bits __read_mostly;
  * s390 does this to prevent multiplexing of hardware bits
  * related to the physical page in case of virtualization.
  */
-#ifndef mm_forbids_zeropage
-#define mm_forbids_zeropage(X)	(0)
+#ifndef vma_forbids_zeropage
+#define vma_forbids_zeropage(vma)	vma_keyid(vma)
 #endif
 
 /*
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 1cd7c1a57a14..83f096c7299b 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -676,8 +676,7 @@  int do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 		return VM_FAULT_OOM;
 	if (unlikely(khugepaged_enter(vma, vma->vm_flags)))
 		return VM_FAULT_OOM;
-	if (!(vmf->flags & FAULT_FLAG_WRITE) &&
-			!mm_forbids_zeropage(vma->vm_mm) &&
+	if (!(vmf->flags & FAULT_FLAG_WRITE) && !vma_forbids_zeropage(vma) &&
 			transparent_hugepage_use_zero_page()) {
 		pgtable_t pgtable;
 		struct page *zero_page;
diff --git a/mm/memory.c b/mm/memory.c
index 02fbef2bd024..a705637d2ded 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3139,8 +3139,7 @@  static int do_anonymous_page(struct vm_fault *vmf)
 		return 0;
 
 	/* Use the zero-page for reads */
-	if (!(vmf->flags & FAULT_FLAG_WRITE) &&
-			!mm_forbids_zeropage(vma->vm_mm)) {
+	if (!(vmf->flags & FAULT_FLAG_WRITE) && !vma_forbids_zeropage(vma)) {
 		entry = pte_mkspecial(pfn_pte(my_zero_pfn(vmf->address),
 						vma->vm_page_prot));
 		vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,