From: Tom Lendacky <[email protected]>
Implement the callbacks to copy the processor state required by KVM to
the GHCB.
Signed-off-by: Tom Lendacky <[email protected]>
[ [email protected]: - Split out of a larger patch
- Adapt to different callback functions ]
Co-developed-by: Joerg Roedel <[email protected]>
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/kernel/kvm.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 08320b0b2b27..0f9597275e9c 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -36,6 +36,8 @@
#include <asm/hypervisor.h>
#include <asm/tlb.h>
#include <asm/cpuidle_haltpoll.h>
+#include <asm/ptrace.h>
+#include <asm/svm.h>
DEFINE_STATIC_KEY_FALSE(kvm_async_pf_enabled);
@@ -746,13 +748,34 @@ static void __init kvm_init_platform(void)
x86_platform.apic_post_init = kvm_apic_init;
}
+#if defined(CONFIG_AMD_MEM_ENCRYPT)
+static void kvm_sev_es_hcall_prepare(struct ghcb *ghcb, struct pt_regs *regs)
+{
+ /* RAX and CPL are already in the GHCB */
+ ghcb_set_rbx(ghcb, regs->bx);
+ ghcb_set_rcx(ghcb, regs->cx);
+ ghcb_set_rdx(ghcb, regs->dx);
+ ghcb_set_rsi(ghcb, regs->si);
+}
+
+static bool kvm_sev_es_hcall_finish(struct ghcb *ghcb, struct pt_regs *regs)
+{
+ /* No checking of the return state needed */
+ return true;
+}
+#endif
+
const __initconst struct hypervisor_x86 x86_hyper_kvm = {
- .name = "KVM",
- .detect = kvm_detect,
- .type = X86_HYPER_KVM,
- .init.guest_late_init = kvm_guest_init,
- .init.x2apic_available = kvm_para_available,
- .init.init_platform = kvm_init_platform,
+ .name = "KVM",
+ .detect = kvm_detect,
+ .type = X86_HYPER_KVM,
+ .init.guest_late_init = kvm_guest_init,
+ .init.x2apic_available = kvm_para_available,
+ .init.init_platform = kvm_init_platform,
+#if defined(CONFIG_AMD_MEM_ENCRYPT)
+ .runtime.sev_es_hcall_prepare = kvm_sev_es_hcall_prepare,
+ .runtime.sev_es_hcall_finish = kvm_sev_es_hcall_finish,
+#endif
};
static __init int activate_jump_labels(void)
--
2.28.0
The following commit has been merged into the x86/seves branch of tip:
Commit-ID: 99419b251e5427b89dbfae103d8a2f469efaa4b2
Gitweb: https://git.kernel.org/tip/99419b251e5427b89dbfae103d8a2f469efaa4b2
Author: Tom Lendacky <[email protected]>
AuthorDate: Mon, 07 Sep 2020 15:16:04 +02:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Wed, 09 Sep 2020 11:33:20 +02:00
x86/kvm: Add KVM-specific VMMCALL handling under SEV-ES
Implement the callbacks to copy the processor state required by KVM to
the GHCB.
Signed-off-by: Tom Lendacky <[email protected]>
[ [email protected]: - Split out of a larger patch
- Adapt to different callback functions ]
Co-developed-by: Joerg Roedel <[email protected]>
Signed-off-by: Joerg Roedel <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/kvm.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 08320b0..0f95972 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -36,6 +36,8 @@
#include <asm/hypervisor.h>
#include <asm/tlb.h>
#include <asm/cpuidle_haltpoll.h>
+#include <asm/ptrace.h>
+#include <asm/svm.h>
DEFINE_STATIC_KEY_FALSE(kvm_async_pf_enabled);
@@ -746,13 +748,34 @@ static void __init kvm_init_platform(void)
x86_platform.apic_post_init = kvm_apic_init;
}
+#if defined(CONFIG_AMD_MEM_ENCRYPT)
+static void kvm_sev_es_hcall_prepare(struct ghcb *ghcb, struct pt_regs *regs)
+{
+ /* RAX and CPL are already in the GHCB */
+ ghcb_set_rbx(ghcb, regs->bx);
+ ghcb_set_rcx(ghcb, regs->cx);
+ ghcb_set_rdx(ghcb, regs->dx);
+ ghcb_set_rsi(ghcb, regs->si);
+}
+
+static bool kvm_sev_es_hcall_finish(struct ghcb *ghcb, struct pt_regs *regs)
+{
+ /* No checking of the return state needed */
+ return true;
+}
+#endif
+
const __initconst struct hypervisor_x86 x86_hyper_kvm = {
- .name = "KVM",
- .detect = kvm_detect,
- .type = X86_HYPER_KVM,
- .init.guest_late_init = kvm_guest_init,
- .init.x2apic_available = kvm_para_available,
- .init.init_platform = kvm_init_platform,
+ .name = "KVM",
+ .detect = kvm_detect,
+ .type = X86_HYPER_KVM,
+ .init.guest_late_init = kvm_guest_init,
+ .init.x2apic_available = kvm_para_available,
+ .init.init_platform = kvm_init_platform,
+#if defined(CONFIG_AMD_MEM_ENCRYPT)
+ .runtime.sev_es_hcall_prepare = kvm_sev_es_hcall_prepare,
+ .runtime.sev_es_hcall_finish = kvm_sev_es_hcall_finish,
+#endif
};
static __init int activate_jump_labels(void)
[resending in plain/text, sorry for double sending]
It seems to me that the kvm_sev_es_hcall_prepare is leaking more
information than it is needed. Is this an expected behavior?
-Erdem
>
> On Thu, Sep 10, 2020 at 2:39 AM tip-bot2 for Tom Lendacky <[email protected]> wrote:
>>
>> The following commit has been merged into the x86/seves branch of tip:
>>
>> Commit-ID: 99419b251e5427b89dbfae103d8a2f469efaa4b2
>> Gitweb: https://git.kernel.org/tip/99419b251e5427b89dbfae103d8a2f469efaa4b2
>> Author: Tom Lendacky <[email protected]>
>> AuthorDate: Mon, 07 Sep 2020 15:16:04 +02:00
>> Committer: Borislav Petkov <[email protected]>
>> CommitterDate: Wed, 09 Sep 2020 11:33:20 +02:00
>>
>> x86/kvm: Add KVM-specific VMMCALL handling under SEV-ES
>>
>> Implement the callbacks to copy the processor state required by KVM to
>> the GHCB.
>>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> [ [email protected]: - Split out of a larger patch
>> - Adapt to different callback functions ]
>> Co-developed-by: Joerg Roedel <[email protected]>
>> Signed-off-by: Joerg Roedel <[email protected]>
>> Signed-off-by: Borislav Petkov <[email protected]>
>> Link: https://lkml.kernel.org/r/[email protected]
>> ---
>> arch/x86/kernel/kvm.c | 35 +++++++++++++++++++++++++++++------
>> 1 file changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 08320b0..0f95972 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -36,6 +36,8 @@
>> #include <asm/hypervisor.h>
>> #include <asm/tlb.h>
>> #include <asm/cpuidle_haltpoll.h>
>> +#include <asm/ptrace.h>
>> +#include <asm/svm.h>
>>
>> DEFINE_STATIC_KEY_FALSE(kvm_async_pf_enabled);
>>
>> @@ -746,13 +748,34 @@ static void __init kvm_init_platform(void)
>> x86_platform.apic_post_init = kvm_apic_init;
>> }
>>
>> +#if defined(CONFIG_AMD_MEM_ENCRYPT)
>> +static void kvm_sev_es_hcall_prepare(struct ghcb *ghcb, struct pt_regs *regs)
>> +{
>> + /* RAX and CPL are already in the GHCB */
>> + ghcb_set_rbx(ghcb, regs->bx);
>> + ghcb_set_rcx(ghcb, regs->cx);
>> + ghcb_set_rdx(ghcb, regs->dx);
>> + ghcb_set_rsi(ghcb, regs->si);
>> +}
>> +
>> +static bool kvm_sev_es_hcall_finish(struct ghcb *ghcb, struct pt_regs *regs)
>> +{
>> + /* No checking of the return state needed */
>> + return true;
>> +}
>> +#endif
>> +
>> const __initconst struct hypervisor_x86 x86_hyper_kvm = {
>> - .name = "KVM",
>> - .detect = kvm_detect,
>> - .type = X86_HYPER_KVM,
>> - .init.guest_late_init = kvm_guest_init,
>> - .init.x2apic_available = kvm_para_available,
>> - .init.init_platform = kvm_init_platform,
>> + .name = "KVM",
>> + .detect = kvm_detect,
>> + .type = X86_HYPER_KVM,
>> + .init.guest_late_init = kvm_guest_init,
>> + .init.x2apic_available = kvm_para_available,
>> + .init.init_platform = kvm_init_platform,
>> +#if defined(CONFIG_AMD_MEM_ENCRYPT)
>> + .runtime.sev_es_hcall_prepare = kvm_sev_es_hcall_prepare,
>> + .runtime.sev_es_hcall_finish = kvm_sev_es_hcall_finish,
>> +#endif
>> };
>>
>> static __init int activate_jump_labels(void)
On Tue, Oct 27, 2020 at 04:14:15PM -0700, Erdem Aktas wrote:
> It seems to me that the kvm_sev_es_hcall_prepare is leaking more
> information than it is needed. Is this an expected behavior?
What exactly is leaked? The kvm hypercall uses RAX, RBX, RCX, RDX and
RSI for parameters.
Regards,
Joerg
I might be missing something here but I think what you say is only
correct for the kvm_hypercall4 cases. All other functions use a
smaller number of registers. #VC blindly assumes that all those
registers are used in the vmcall and exposes them. Here are some
examples:
For example in the kvm_hypercall2 only rax, rbx, and rcx should be
exposed. apic_id address is leaked with rdx when this hypercall is
used in kvm_kick_cpu function. RSI is never used. I am not sure what
value will be exposed to VMM in this case:
54 static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
55 unsigned long p2)
56 {
57 long ret;
58 asm volatile(KVM_HYPERCALL
59 : "=a"(ret)
60 : "a"(nr), "b"(p1), "c"(p2)
61 : "memory");
62 return ret;
63 }
And this function is called in :
820 static void kvm_kick_cpu(int cpu)
821 {
822 int apicid;
823 unsigned long flags = 0;
824
825 apicid = per_cpu(x86_cpu_to_apicid, cpu);
826 kvm_hypercall2(KVM_HC_KICK_CPU, flags, apicid);
827 }
looking to what it is compiled in my machine :
151215 ffffffff8105def0 <kvm_kick_cpu>:
151216 {
151217 ffffffff8105def0: e8 fb 9e ff ff callq
ffffffff81057df0 <__fentry__>
151218 apicid = per_cpu(x86_cpu_to_apicid, cpu);
151219 ffffffff8105def5: 48 63 ff movslq %edi,%rdi
151220 {
151221 ffffffff8105def8: 53 push %rbx
151222 apicid = per_cpu(x86_cpu_to_apicid, cpu);
151223 ffffffff8105def9: 48 c7 c0 58 16 01 00 mov $0x11658,%rax
151224
151225 static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
151226 unsigned long p2)
151227 {
151228 long ret;
151229 asm volatile(KVM_HYPERCALL
151230 ffffffff8105df00: 31 db xor %ebx,%ebx
151231 ffffffff8105df02: 48 8b 14 fd 00 19 cb mov
-0x7e34e700(,%rdi,8),%rdx
151232 ffffffff8105df09: 81
151233 kvm_hypercall2(KVM_HC_KICK_CPU, flags, apicid);
151234 ffffffff8105df0a: 0f b7 0c 02 movzwl
(%rdx,%rax,1),%ecx
151235 ffffffff8105df0e: b8 05 00 00 00 mov $0x5,%eax
151236 ffffffff8105df13: 0f 01 c1 vmcall
151237 }
151238 ffffffff8105df16: 5b pop %rbx
151239 ffffffff8105df17: c3 retq
Similarly kvm_hypercall1 only need 2 registers to expose:
44 static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
45 {
46 long ret;
47 asm volatile(KVM_HYPERCALL
48 : "=a"(ret)
49 : "a"(nr), "b"(p1)
50 : "memory");
51 return ret;
52 }
And an example where it is used:
562 static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
563 {
564 int cpu;
565
566 native_send_call_func_ipi(mask);
567
568 /* Make sure other vCPUs get a chance to run if they need to. */
569 for_each_cpu(cpu, mask) {
570 if (vcpu_is_preempted(cpu)) {
571 kvm_hypercall1(KVM_HC_SCHED_YIELD,
per_cpu(x86_cpu_to_apicid, cpu));
572 break;
573 }
574 }
575 }
If we look at the function decompiled in my platform, here
x86_cpu_to_apicid address is leaked in rdx. RSI also leaks some
information from kvm_smp_send_call_function_ipi function. RCX is not
used so it might include something from a higher caller.
151243 ffffffff8105df20 <kvm_smp_send_call_func_ipi>:
151244 {
151245 ffffffff8105df20: e8 cb 9e ff ff callq
ffffffff81057df0 <__fentry__>
151246 ffffffff8105df25: 53 push %rbx
151247 ffffffff8105df26: 48 89 fb mov %rdi,%rbx
151248 native_send_call_func_ipi(mask);
151249 ffffffff8105df29: e8 a2 45 ff ff callq
ffffffff810524d0 <native_send_call_func_ipi>
151250 for_each_cpu(cpu, mask) {
151251 ffffffff8105df2e: 41 b8 ff ff ff ff mov $0xffffffff,%r8d
151252 ffffffff8105df34: eb 0e jmp
ffffffff8105df44 <kvm_smp_send_call_func_ipi+0x24>
151253 return PVOP_CALLEE1(bool, lock.vcpu_is_preempted, cpu);
151254 ffffffff8105df36: 49 63 f8 movslq %r8d,%rdi
151255 ffffffff8105df39: ff 14 25 90 93 02 82 callq
*0xffffffff82029390
151256 if (vcpu_is_preempted(cpu)) {
151257 ffffffff8105df40: 84 c0 test %al,%al
151258 ffffffff8105df42: 75 18 jne
ffffffff8105df5c <kvm_smp_send_call_func_ipi+0x3c>
151259 for_each_cpu(cpu, mask) {
151260 ffffffff8105df44: 44 89 c7 mov %r8d,%edi
151261 ffffffff8105df47: 48 89 de mov %rbx,%rsi
151262 ffffffff8105df4a: e8 61 44 39 00 callq
ffffffff813f23b0 <cpumask_next>
151263 ffffffff8105df4f: 3b 05 2f 7e 12 01 cmp
0x1127e2f(%rip),%eax # ffffffff82185d84 <nr_cpu_ids>
151264 ffffffff8105df55: 41 89 c0 mov %eax,%r8d
151265 ffffffff8105df58: 72 dc jb
ffffffff8105df36 <kvm_smp_send_call_func_ipi+0x16>
151266 }
151267 ffffffff8105df5a: 5b pop %rbx
151268 ffffffff8105df5b: c3 retq
151269 kvm_hypercall1(KVM_HC_SCHED_YIELD,
per_cpu(x86_cpu_to_apicid, cpu));
151270 ffffffff8105df5c: 48 8b 14 fd 00 19 cb mov
-0x7e34e700(,%rdi,8),%rdx
151271 ffffffff8105df63: 81
151272 ffffffff8105df64: 48 c7 c0 58 16 01 00 mov $0x11658,%rax
151273 ffffffff8105df6b: 0f b7 1c 02 movzwl
(%rdx,%rax,1),%ebx
151274 asm volatile(KVM_HYPERCALL
151275 ffffffff8105df6f: b8 0b 00 00 00 mov $0xb,%eax
151276 ffffffff8105df74: 0f 01 c1 vmcall
151277 }
I am not sure how those leaked registers can be used, but depending on
which function call hypercall[0-3], there will be some leak.
-Erdem
On Wed, Oct 28, 2020 at 2:49 AM Joerg Roedel <[email protected]> wrote:
>
> On Tue, Oct 27, 2020 at 04:14:15PM -0700, Erdem Aktas wrote:
> > It seems to me that the kvm_sev_es_hcall_prepare is leaking more
> > information than it is needed. Is this an expected behavior?
>
> What exactly is leaked? The kvm hypercall uses RAX, RBX, RCX, RDX and
> RSI for parameters.
>
> Regards,
>
> Joerg
On Wed, Oct 28, 2020 at 11:03:05AM -0700, Erdem Aktas wrote:
> I might be missing something here but I think what you say is only
> correct for the kvm_hypercall4 cases. All other functions use a
> smaller number of registers. #VC blindly assumes that all those
> registers are used in the vmcall and exposes them.
Right, I think we should fix that in the guest and zero out the unused
registers. VMMCALL can come from userspace after all, and the #VC
handler does not look at the hypercall numbers.
Further, on the host side KVM will unconditionally read out all 4
registers too, which requires us to set them valid in the GHCB.
Regards,
Joerg