diff mbox

[PATCHv5,03/19] mm/ksm: Do not merge pages with different KeyIDs

Message ID 20180717112029.42378-4-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
Pages encrypted with different encryption keys are not allowed to be
merged by KSM. 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 July 18, 2018, 5:38 p.m. UTC | #1
On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> Pages encrypted with different encryption keys are not allowed to be
> merged by KSM. Otherwise it would cross security boundary.

Let's say I'm using plain AES (not AES-XTS).  I use the same key in two
keyid slots.  I map a page with the first keyid and another with the
other keyid.

Won't they have the same cipertext?  Why shouldn't we KSM them?
Kirill A. Shutemov July 19, 2018, 7:32 a.m. UTC | #2
On Wed, Jul 18, 2018 at 10:38:27AM -0700, Dave Hansen wrote:
> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> > Pages encrypted with different encryption keys are not allowed to be
> > merged by KSM. Otherwise it would cross security boundary.
> 
> Let's say I'm using plain AES (not AES-XTS).  I use the same key in two
> keyid slots.  I map a page with the first keyid and another with the
> other keyid.
> 
> Won't they have the same cipertext?  Why shouldn't we KSM them?

We compare plain text, not ciphertext. And for good reason.

Comparing ciphertext would only make KSM successful for AES-ECB that
doesn't dependent on physical address of the page.

MKTME only supports AES-XTS (no plans to support AES-ECB). It effectively
disables KSM if we go with comparing ciphertext.
Dave Hansen July 19, 2018, 2:02 p.m. UTC | #3
On 07/19/2018 12:32 AM, Kirill A. Shutemov wrote:
> On Wed, Jul 18, 2018 at 10:38:27AM -0700, Dave Hansen wrote:
>> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
>>> Pages encrypted with different encryption keys are not allowed to be
>>> merged by KSM. Otherwise it would cross security boundary.
>> Let's say I'm using plain AES (not AES-XTS).  I use the same key in two
>> keyid slots.  I map a page with the first keyid and another with the
>> other keyid.
>>
>> Won't they have the same cipertext?  Why shouldn't we KSM them?
> We compare plain text, not ciphertext. And for good reason.

What's the reason?  Probably good to talk about it for those playing
along at home.

> Comparing ciphertext would only make KSM successful for AES-ECB that
> doesn't dependent on physical address of the page.
> 
> MKTME only supports AES-XTS (no plans to support AES-ECB). It effectively
> disables KSM if we go with comparing ciphertext.

But what's the security boundary that is violated?  You are talking
about some practical concerns (KSM scanning inefficiency) which is a far
cry from being any kind of security issue.
Kirill A. Shutemov July 20, 2018, 12:23 p.m. UTC | #4
On Thu, Jul 19, 2018 at 07:02:34AM -0700, Dave Hansen wrote:
> On 07/19/2018 12:32 AM, Kirill A. Shutemov wrote:
> > On Wed, Jul 18, 2018 at 10:38:27AM -0700, Dave Hansen wrote:
> >> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> >>> Pages encrypted with different encryption keys are not allowed to be
> >>> merged by KSM. Otherwise it would cross security boundary.
> >> Let's say I'm using plain AES (not AES-XTS).  I use the same key in two
> >> keyid slots.  I map a page with the first keyid and another with the
> >> other keyid.
> >>
> >> Won't they have the same cipertext?  Why shouldn't we KSM them?
> > We compare plain text, not ciphertext. And for good reason.
> 
> What's the reason?  Probably good to talk about it for those playing
> along at home.

I'll update commit message.

> > Comparing ciphertext would only make KSM successful for AES-ECB that
> > doesn't dependent on physical address of the page.
> > 
> > MKTME only supports AES-XTS (no plans to support AES-ECB). It effectively
> > disables KSM if we go with comparing ciphertext.
> 
> But what's the security boundary that is violated?  You are talking
> about some practical concerns (KSM scanning inefficiency) which is a far
> cry from being any kind of security issue.

As with zero page, my initial reasoning was that mixing pages from
different secrutiy domains is bad idea.
diff mbox

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 151d6e6b16e5..a4ce26aa0b65 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1551,6 +1551,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 a6d43cf9a982..1bd7b9710e29 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1214,6 +1214,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