Skip to contentSkip to conversation

about vmx_l1d_flush_pages

MODERATED In review Nicolai Stange 2 participants 5 messages Jul 18, 2018 – Jul 20, 2018

Subsystems (2)
x86 KVM x86
lore
Share & tools
lore

Nicolai Stange

author
#0 lore raw

Hi,

I've got two questions related to the initialization of vmx_l1d_flush_pages from v15 commit a47dd5f06714 ("x86/KVM/VMX: Add L1D flush algorithm").

[Apologies for not replying properly -- I don't have the original mail].

1.) The
[empty_zp] "r" (vmx_l1d_flush_pages)
asm constraint in vmx_l1d_flush() seems to suggest that these pages are zeroed out. But AFAICS they're actually left uninitialized. Am I wrong or is this intended?

2.) With nested KVM, the vmx_l1d_flush_pages could be subject to KSM on
the host. This means that the 16 vmx_l1d_flush_pages could get mapped to fewer host physical pages and that would break the L1d flush?

If so, an obvious fix would be to initialize all 16 pages with a different pattern each.

Thanks,

Nicolai

Signature · 2 lines
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)
#1 lore raw
Quoted 5 lines · speck for Nicolai Stange

I've got two questions related to the initialization of vmx_l1d_flush_pages from v15 commit a47dd5f06714 ("x86/KVM/VMX: Add L1D flush algorithm").

[Apologies for not replying properly -- I don't have the original mail].

Want the archive ?

Quoted 5 lines

1.) The
[empty_zp] "r" (vmx_l1d_flush_pages)
asm constraint in vmx_l1d_flush() seems to suggest that these pages are zeroed out. But AFAICS they're actually left uninitialized. Am I wrong or is this intended?

Not intended AFAICT.

Quoted 4 lines

2.) With nested KVM, the vmx_l1d_flush_pages could be subject to KSM on
the host. This means that the 16 vmx_l1d_flush_pages could get mapped to fewer host physical pages and that would break the L1d flush?

Probably.

Quoted 2 lines

If so, an obvious fix would be to initialize all 16 pages with a different pattern each.

Care to whip up a patch?

Thanks,

tglx

author
#2 lore raw

PATCH 1/1 fix L1TF kvm initialization

From: Nicolai Stange <[email protected]>
Subject: [PATCH] x86/KVM/VMX: initialize the vmx_l1d_flush_pages' content

The slow path in vmx_l1d_flush() reads from vmx_l1d_flush_pages in order to evict the L1d cache.

However, these are never cleared and, in theory, their data could be leaked.

More importantly, KSM could merge a nested hypervisor's vmx_l1d_flush_pages to fewer than 1 << L1D_CACHE_ORDER host physical pages and this would break the L1d flushing algorithm: L1d on x86_64 is tagged by physical addresses.

Fix this by initializing the individual vmx_l1d_flush_pages with a different pattern each.

Rename the "empty_zp" asm constraint identifier in vmx_l1d_flush() to "flush_pages" to reflect this change.

Signed-off-by Nicolai Stange <[email protected]>
---
 arch/x86/kvm/vmx.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
arch/x86/kvm/vmx.c +14 -3
index c5c0118b126d..b4b8e8cb4a7e 100644
@@ -211,6 +211,7 @@ static void *vmx_l1d_flush_pages;
 static int vmx_setup_l1d_flush(enum vmx_l1d_flush_state l1tf)
 {
 	struct page *page;
+	unsigned int i;
 
 	if (!enable_ept) {
 		l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_EPT_DISABLED;

@@ -243,6 +244,16 @@ static int vmx_setup_l1d_flush(enum vmx_l1d_flush_state l1tf)
 		if (!page)
 			return -ENOMEM;
 		vmx_l1d_flush_pages = page_address(page);
+
+		/*
+		 * Initialize each page with a different pattern in
+		 * order to protect against KSM in the nested
+		 * virtualization case.
+		 */
+		for (i = 0; i < 1u << L1D_CACHE_ORDER; ++i) {
+			memset(vmx_l1d_flush_pages + i * PAGE_SIZE, i + 1,
+			       PAGE_SIZE);
+		}
 	}
 
 	l1tf_vmx_mitigation = l1tf;

@@ -9701,7 +9712,7 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
 		/* First ensure the pages are in the TLB */
 		"xorl	%%eax, %%eax\n"
 		".Lpopulate_tlb:\n\t"
-		"movzbl	(%[empty_zp], %%" _ASM_AX "), %%ecx\n\t"
+		"movzbl	(%[flush_pages], %%" _ASM_AX "), %%ecx\n\t"
 		"addl	$4096, %%eax\n\t"
 		"cmpl	%%eax, %[size]\n\t"
 		"jne	.Lpopulate_tlb\n\t"

@@ -9710,12 +9721,12 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
 		/* Now fill the cache */
 		"xorl	%%eax, %%eax\n"
 		".Lfill_cache:\n"
-		"movzbl	(%[empty_zp], %%" _ASM_AX "), %%ecx\n\t"
+		"movzbl	(%[flush_pages], %%" _ASM_AX "), %%ecx\n\t"
 		"addl	$64, %%eax\n\t"
 		"cmpl	%%eax, %[size]\n\t"
 		"jne	.Lfill_cache\n\t"
 		"lfence\n"
-		:: [empty_zp] "r" (vmx_l1d_flush_pages),
+		:: [flush_pages] "r" (vmx_l1d_flush_pages),
 		    [size] "r" (size)
 		: "eax", "ebx", "ecx", "edx");
 }
Signature · 1 line
2.13.7
#3 lore raw

Re: [PATCH] fix L1TF kvm initialization

Quoted 12 lines · speck for Nicolai Stange

From: Nicolai Stange <[email protected]>
Subject: [PATCH] x86/KVM/VMX: initialize the vmx_l1d_flush_pages' content

The slow path in vmx_l1d_flush() reads from vmx_l1d_flush_pages in order to evict the L1d cache.

However, these are never cleared and, in theory, their data could be leaked.

More importantly, KSM could merge a nested hypervisor's vmx_l1d_flush_pages to fewer than 1 << L1D_CACHE_ORDER host physical pages and this would break the L1d flushing algorithm: L1d on x86_64 is tagged by physical addresses.

Quoted 7 lines

Fix this by initializing the individual vmx_l1d_flush_pages with a different pattern each.

Rename the "empty_zp" asm constraint identifier in vmx_l1d_flush() to "flush_pages" to reflect this change.

Signed-off-by Nicolai Stange <[email protected]>

Applied and all branches updated.

Thanks,

tglx

author
#4 lore raw
In reply to Thomas Gleixner
Quoted 8 lines · speck for Thomas Gleixner
Quoted 5 lines · speck for Nicolai Stange

I've got two questions related to the initialization of vmx_l1d_flush_pages from v15 commit a47dd5f06714 ("x86/KVM/VMX: Add L1D flush algorithm").

[Apologies for not replying properly -- I don't have the original mail].

Want the archive ?

That would be great, thanks for the offer!

Nicolai

Signature · 2 lines
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)