diff mbox

[PATCHv3,03/17] mm/ksm: Do not merge pages with different KeyIDs

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

Commit Message

Kirill A. Shutemov June 12, 2018, 2:39 p.m. UTC
Pages encrypted with different encryption keys are not subject to KSM
merge. Otherwise it would cross security boundary.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/mm.h | 7 +++++++
 mm/ksm.c           | 3 +++
 2 files changed, 10 insertions(+)

Comments

Dave Hansen June 13, 2018, 5:51 p.m. UTC | #1
On 06/12/2018 07:39 AM, Kirill A. Shutemov wrote:
> Pages encrypted with different encryption keys are not subject to KSM
> merge. Otherwise it would cross security boundary.

This needs a much stronger explanation.  Which KeyID would be used for
access in the new direct mappings?  What actually happens without this
patch in place?
Kirill A. Shutemov June 13, 2018, 8:31 p.m. UTC | #2
On Wed, Jun 13, 2018 at 05:51:50PM +0000, Dave Hansen wrote:
> On 06/12/2018 07:39 AM, Kirill A. Shutemov wrote:
> > Pages encrypted with different encryption keys are not subject to KSM
> > merge. Otherwise it would cross security boundary.
> 
> This needs a much stronger explanation.

Okay, fair enough.

> Which KeyID would be used for access in the new direct mappings?

New direct mapping?

Pages would be compared using direct mappings relevant for their KeyID.
They will be threated as identical if they plain-text is identical.

> What actually happens without this patch in place?

One of processes would get the page mapped with wrong KeyID and see
garbage. We setup mapping according to KeyID in vma->vm_page_prot.
Dave Hansen June 13, 2018, 8:35 p.m. UTC | #3
On 06/13/2018 01:31 PM, Kirill A. Shutemov wrote:
>> What actually happens without this patch in place?
> 
> One of processes would get the page mapped with wrong KeyID and see
> garbage. 

OK, but what about two pages with the same KeyID?  It's actually totally
possible for KSM to determine that two pages have the same plaintext and
merge them.  Why don't we do that?

> We setup mapping according to KeyID in vma->vm_page_prot.

Then why do we bother with page_keyid() and the page_ext stuff?
Dave Hansen June 13, 2018, 8:40 p.m. UTC | #4
On 06/13/2018 01:35 PM, Dave Hansen wrote:
> On 06/13/2018 01:31 PM, Kirill A. Shutemov wrote:
>>> What actually happens without this patch in place?
>> One of processes would get the page mapped with wrong KeyID and see
>> garbage. 
> OK, but what about two pages with the same KeyID?  It's actually totally
> possible for KSM to determine that two pages have the same plaintext and
> merge them.  Why don't we do that?

/me goes back to look at the patch... which is doing exactly that

But, this still needs a stronger explanation of what goes wrong if this
patch is not in place.
Kirill A. Shutemov June 13, 2018, 8:41 p.m. UTC | #5
On Wed, Jun 13, 2018 at 08:35:46PM +0000, Dave Hansen wrote:
> On 06/13/2018 01:31 PM, Kirill A. Shutemov wrote:
> >> What actually happens without this patch in place?
> > 
> > One of processes would get the page mapped with wrong KeyID and see
> > garbage. 
> 
> OK, but what about two pages with the same KeyID?  It's actually totally
> possible for KSM to determine that two pages have the same plaintext and
> merge them.  Why don't we do that?

That's exactly what we do :)

> > We setup mapping according to KeyID in vma->vm_page_prot.
> 
> Then why do we bother with page_keyid() and the page_ext stuff?

VMA is not always around.

Using KeyID in vma->vm_page_prot we don't need to change anything in PTE
setup functions. It just works.
diff mbox

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 435b053c457c..ac1a8480284d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1506,6 +1506,13 @@  static inline int vma_keyid(struct vm_area_struct *vma)
 }
 #endif
 
+#ifndef page_keyid
+static inline int page_keyid(struct page *page)
+{
+	return 0;
+}
+#endif
+
 #ifdef CONFIG_SHMEM
 /*
  * The vma_is_shmem is not inline because it is used only by slow
diff --git a/mm/ksm.c b/mm/ksm.c
index 7d6558f3bac9..db94bd45fe66 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1201,6 +1201,9 @@  static int try_to_merge_one_page(struct vm_area_struct *vma,
 	if (!PageAnon(page))
 		goto out;
 
+	if (page_keyid(page) != page_keyid(kpage))
+		goto out;
+
 	/*
 	 * We need the page lock to read a stable PageSwapCache in
 	 * write_protect_page().  We use trylock_page() instead of