| Message ID | d0904f0d049300267665bd4abf96c3d7e7aa4825.1621701837.git.thomas.lendacky@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Assume a 64-bit hypercall for guests with protected state | expand |
On 5/22/21 11:43 AM, Tom Lendacky wrote: > When processing a hypercall for a guest with protected state, currently > SEV-ES guests, the guest CS segment register can't be checked to > determine if the guest is in 64-bit mode. For an SEV-ES guest, it is > expected that communication between the guest and the hypervisor is > performed to shared memory using the GHCB. In order to use the GHCB, the > guest must have been in long mode, otherwise writes by the guest to the > GHCB would be encrypted and not be able to be comprehended by the > hypervisor. Given that, assume that the guest is in 64-bit mode when > processing a hypercall from a guest with protected state. > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES") > Reported-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/kvm/x86.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Subject should actually be KVM: x86: ..., not KVM: SVM: Sorry about that. Tom > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 9b6bca616929..e715c69bb882 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > > trace_kvm_hypercall(nr, a0, a1, a2, a3); > > - op_64_bit = is_64_bit_mode(vcpu); > + /* > + * If running with protected guest state, the CS register is not > + * accessible. The hypercall register values will have had to been > + * provided in 64-bit mode, so assume the guest is in 64-bit. > + */ > + op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected; > if (!op_64_bit) { > nr &= 0xFFFFFFFF; > a0 &= 0xFFFFFFFF; >
Tom Lendacky <thomas.lendacky@amd.com> writes: > When processing a hypercall for a guest with protected state, currently > SEV-ES guests, the guest CS segment register can't be checked to > determine if the guest is in 64-bit mode. For an SEV-ES guest, it is > expected that communication between the guest and the hypervisor is > performed to shared memory using the GHCB. In order to use the GHCB, the > guest must have been in long mode, otherwise writes by the guest to the > GHCB would be encrypted and not be able to be comprehended by the > hypervisor. Given that, assume that the guest is in 64-bit mode when > processing a hypercall from a guest with protected state. > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES") > Reported-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/kvm/x86.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 9b6bca616929..e715c69bb882 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > > trace_kvm_hypercall(nr, a0, a1, a2, a3); > > - op_64_bit = is_64_bit_mode(vcpu); > + /* > + * If running with protected guest state, the CS register is not > + * accessible. The hypercall register values will have had to been > + * provided in 64-bit mode, so assume the guest is in 64-bit. > + */ > + op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected; > if (!op_64_bit) { > nr &= 0xFFFFFFFF; > a0 &= 0xFFFFFFFF; While this is might be a very theoretical question, what about other is_64_bit_mode() users? Namely, a very similar to the above check exists in kvm_hv_hypercall() and kvm_xen_hypercall().
On 22/05/21 18:43, Tom Lendacky wrote: > When processing a hypercall for a guest with protected state, currently > SEV-ES guests, the guest CS segment register can't be checked to > determine if the guest is in 64-bit mode. For an SEV-ES guest, it is > expected that communication between the guest and the hypervisor is > performed to shared memory using the GHCB. In order to use the GHCB, the > guest must have been in long mode, otherwise writes by the guest to the > GHCB would be encrypted and not be able to be comprehended by the > hypervisor. Given that, assume that the guest is in 64-bit mode when > processing a hypercall from a guest with protected state. > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES") > Reported-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/kvm/x86.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 9b6bca616929..e715c69bb882 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > > trace_kvm_hypercall(nr, a0, a1, a2, a3); > > - op_64_bit = is_64_bit_mode(vcpu); > + /* > + * If running with protected guest state, the CS register is not > + * accessible. The hypercall register values will have had to been > + * provided in 64-bit mode, so assume the guest is in 64-bit. > + */ > + op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected; > if (!op_64_bit) { > nr &= 0xFFFFFFFF; > a0 &= 0xFFFFFFFF; > Queued, thanks. Paolo
On 5/24/21 6:53 AM, Vitaly Kuznetsov wrote: > Tom Lendacky <thomas.lendacky@amd.com> writes: > >> When processing a hypercall for a guest with protected state, currently >> SEV-ES guests, the guest CS segment register can't be checked to >> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is >> expected that communication between the guest and the hypervisor is >> performed to shared memory using the GHCB. In order to use the GHCB, the >> guest must have been in long mode, otherwise writes by the guest to the >> GHCB would be encrypted and not be able to be comprehended by the >> hypervisor. Given that, assume that the guest is in 64-bit mode when >> processing a hypercall from a guest with protected state. >> >> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES") >> Reported-by: Sean Christopherson <seanjc@google.com> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >> --- >> arch/x86/kvm/x86.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 9b6bca616929..e715c69bb882 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) >> >> trace_kvm_hypercall(nr, a0, a1, a2, a3); >> >> - op_64_bit = is_64_bit_mode(vcpu); >> + /* >> + * If running with protected guest state, the CS register is not >> + * accessible. The hypercall register values will have had to been >> + * provided in 64-bit mode, so assume the guest is in 64-bit. >> + */ >> + op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected; >> if (!op_64_bit) { >> nr &= 0xFFFFFFFF; >> a0 &= 0xFFFFFFFF; > > While this is might be a very theoretical question, what about other > is_64_bit_mode() users? Namely, a very similar to the above check exists > in kvm_hv_hypercall() and kvm_xen_hypercall(). Xen doesn't support SEV, so I think this one is ok until they do. Although I guess we could be preemptive and hit all those call sites. The other ones are in arch/x86/kvm/hyperv.c. Thoughts? Thanks, Tom >
Tom Lendacky <thomas.lendacky@amd.com> writes: > On 5/24/21 6:53 AM, Vitaly Kuznetsov wrote: >> Tom Lendacky <thomas.lendacky@amd.com> writes: >> >>> When processing a hypercall for a guest with protected state, currently >>> SEV-ES guests, the guest CS segment register can't be checked to >>> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is >>> expected that communication between the guest and the hypervisor is >>> performed to shared memory using the GHCB. In order to use the GHCB, the >>> guest must have been in long mode, otherwise writes by the guest to the >>> GHCB would be encrypted and not be able to be comprehended by the >>> hypervisor. Given that, assume that the guest is in 64-bit mode when >>> processing a hypercall from a guest with protected state. >>> >>> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES") >>> Reported-by: Sean Christopherson <seanjc@google.com> >>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >>> --- >>> arch/x86/kvm/x86.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 9b6bca616929..e715c69bb882 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) >>> >>> trace_kvm_hypercall(nr, a0, a1, a2, a3); >>> >>> - op_64_bit = is_64_bit_mode(vcpu); >>> + /* >>> + * If running with protected guest state, the CS register is not >>> + * accessible. The hypercall register values will have had to been >>> + * provided in 64-bit mode, so assume the guest is in 64-bit. >>> + */ >>> + op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected; >>> if (!op_64_bit) { >>> nr &= 0xFFFFFFFF; >>> a0 &= 0xFFFFFFFF; >> >> While this is might be a very theoretical question, what about other >> is_64_bit_mode() users? Namely, a very similar to the above check exists >> in kvm_hv_hypercall() and kvm_xen_hypercall(). > > Xen doesn't support SEV, so I think this one is ok until they do. Although > I guess we could be preemptive and hit all those call sites. The other > ones are in arch/x86/kvm/hyperv.c. > > Thoughts? Would it hurt if we just move 'vcpu->arch.guest_state_protected' check to is_64_bit_mode() itself? It seems to be too easy to miss this peculiar detail about SEV in review if new is_64_bit_mode() users are to be added.
On 5/24/21 8:49 AM, Vitaly Kuznetsov wrote: > Tom Lendacky <thomas.lendacky@amd.com> writes: > >> On 5/24/21 6:53 AM, Vitaly Kuznetsov wrote: >>> Tom Lendacky <thomas.lendacky@amd.com> writes: >>> >>>> When processing a hypercall for a guest with protected state, currently >>>> SEV-ES guests, the guest CS segment register can't be checked to >>>> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is >>>> expected that communication between the guest and the hypervisor is >>>> performed to shared memory using the GHCB. In order to use the GHCB, the >>>> guest must have been in long mode, otherwise writes by the guest to the >>>> GHCB would be encrypted and not be able to be comprehended by the >>>> hypervisor. Given that, assume that the guest is in 64-bit mode when >>>> processing a hypercall from a guest with protected state. >>>> >>>> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES") >>>> Reported-by: Sean Christopherson <seanjc@google.com> >>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >>>> --- >>>> arch/x86/kvm/x86.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>> index 9b6bca616929..e715c69bb882 100644 >>>> --- a/arch/x86/kvm/x86.c >>>> +++ b/arch/x86/kvm/x86.c >>>> @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) >>>> >>>> trace_kvm_hypercall(nr, a0, a1, a2, a3); >>>> >>>> - op_64_bit = is_64_bit_mode(vcpu); >>>> + /* >>>> + * If running with protected guest state, the CS register is not >>>> + * accessible. The hypercall register values will have had to been >>>> + * provided in 64-bit mode, so assume the guest is in 64-bit. >>>> + */ >>>> + op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected; >>>> if (!op_64_bit) { >>>> nr &= 0xFFFFFFFF; >>>> a0 &= 0xFFFFFFFF; >>> >>> While this is might be a very theoretical question, what about other >>> is_64_bit_mode() users? Namely, a very similar to the above check exists >>> in kvm_hv_hypercall() and kvm_xen_hypercall(). >> >> Xen doesn't support SEV, so I think this one is ok until they do. Although >> I guess we could be preemptive and hit all those call sites. The other >> ones are in arch/x86/kvm/hyperv.c. >> >> Thoughts? > > Would it hurt if we just move 'vcpu->arch.guest_state_protected' check > to is_64_bit_mode() itself? It seems to be too easy to miss this > peculiar detail about SEV in review if new is_64_bit_mode() users are to > be added. I thought about that, but wondered if is_64_bit_mode() was to be used in other places in the future, if it would be a concern. I think it would be safe since anyone adding it to a new section of code is likely to look at what that function is doing first. I'm ok with this. Paolo, I know you already queued this, but would you prefer moving the check into is_64_bit_mode()? Thanks, Tom >
On 24/05/21 15:58, Tom Lendacky wrote: >> Would it hurt if we just move 'vcpu->arch.guest_state_protected' check >> to is_64_bit_mode() itself? It seems to be too easy to miss this >> peculiar detail about SEV in review if new is_64_bit_mode() users are to >> be added. > I thought about that, but wondered if is_64_bit_mode() was to be used in > other places in the future, if it would be a concern. I think it would be > safe since anyone adding it to a new section of code is likely to look at > what that function is doing first. > > I'm ok with this. Paolo, I know you already queued this, but would you > prefer moving the check into is_64_bit_mode()? Let's introduce a new wrapper is_64_bit_hypercall, and add a WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode. Paolo
On 5/24/21 9:20 AM, Paolo Bonzini wrote: > On 24/05/21 15:58, Tom Lendacky wrote: >>> Would it hurt if we just move 'vcpu->arch.guest_state_protected' check >>> to is_64_bit_mode() itself? It seems to be too easy to miss this >>> peculiar detail about SEV in review if new is_64_bit_mode() users are to >>> be added. >> I thought about that, but wondered if is_64_bit_mode() was to be used in >> other places in the future, if it would be a concern. I think it would be >> safe since anyone adding it to a new section of code is likely to look at >> what that function is doing first. >> >> I'm ok with this. Paolo, I know you already queued this, but would you >> prefer moving the check into is_64_bit_mode()? > > Let's introduce a new wrapper is_64_bit_hypercall, and add a > WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode. Will do. Thanks, Tom > > Paolo >
On Mon, May 24, 2021, Paolo Bonzini wrote: > On 24/05/21 15:58, Tom Lendacky wrote: > > > Would it hurt if we just move 'vcpu->arch.guest_state_protected' check > > > to is_64_bit_mode() itself? It seems to be too easy to miss this > > > peculiar detail about SEV in review if new is_64_bit_mode() users are to > > > be added. > > I thought about that, but wondered if is_64_bit_mode() was to be used in > > other places in the future, if it would be a concern. I think it would be > > safe since anyone adding it to a new section of code is likely to look at > > what that function is doing first. > > > > I'm ok with this. Paolo, I know you already queued this, but would you > > prefer moving the check into is_64_bit_mode()? > > Let's introduce a new wrapper is_64_bit_hypercall, and add a > WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode. Can we introduce the WARN(s) in a separate patch, and deploy them much more widely than just is_64_bit_mode()? I would like to have them lying in wait at every path that should be unreachable, e.g. get/set segments, get_cpl(), etc... Side topic, kvm_get_cs_db_l_bits() should be moved to svm.c. Functionally, it's fine to have it as a vendor-agnostic helper, but practically speaking it should never be called directly.
On 24/05/21 19:03, Sean Christopherson wrote: >> Let's introduce a new wrapper is_64_bit_hypercall, and add a >> WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode. > > Can we introduce the WARN(s) in a separate patch, and deploy them much more > widely than just is_64_bit_mode()? I would like to have them lying in wait at > every path that should be unreachable, e.g. get/set segments, get_cpl(), etc... Each WARN that is added must be audited separately, so this one I'd like to have now; it is pretty much the motivation for introducing a new function, as the other caller of is_64_bit_mode, kvm_get_linear_rip() is already "handling" SEV-ES by always returning 0. But yes adding more WARNs can only be good. Paolo > Side topic, kvm_get_cs_db_l_bits() should be moved to svm.c. Functionally, it's > fine to have it as a vendor-agnostic helper, but practically speaking it should > never be called directly. >
On 5/24/21 12:06 PM, Paolo Bonzini wrote: > On 24/05/21 19:03, Sean Christopherson wrote: >>> Let's introduce a new wrapper is_64_bit_hypercall, and add a >>> WARN_ON_ONCE(vcpu->arch.guest_state_protected) to is_64_bit_mode. >> >> Can we introduce the WARN(s) in a separate patch, and deploy them much more >> widely than just is_64_bit_mode()? I would like to have them lying in >> wait at >> every path that should be unreachable, e.g. get/set segments, get_cpl(), >> etc... > > Each WARN that is added must be audited separately, so this one I'd like > to have now; it is pretty much the motivation for introducing a new > function, as the other caller of is_64_bit_mode, kvm_get_linear_rip() is > already "handling" SEV-ES by always returning 0. The kvm_register_{read,write}() functions also call is_64_bit_mode(). But... The SVM support uses those for CR and DR intercepts. CR intercepts are not enabled under SEV-ES. DR intercepts are only set for DR7. DR7 reads don't actually exit, the intercept is there to trigger the #VC and return a cached value from the #VC handler. DR7 writes do exit but don't actually do much since we don't allow guest_debug to be set so kvm_register_read() is never called. The x86 support also uses those functions for emulation and SMM, both of which aren't supported under SEV-ES. Thanks, Tom > > But yes adding more WARNs can only be good. > > Paolo > >> Side topic, kvm_get_cs_db_l_bits() should be moved to svm.c. >> Functionally, it's >> fine to have it as a vendor-agnostic helper, but practically speaking it >> should >> never be called directly. >> >
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 9b6bca616929..e715c69bb882 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -8403,7 +8403,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) trace_kvm_hypercall(nr, a0, a1, a2, a3); - op_64_bit = is_64_bit_mode(vcpu); + /* + * If running with protected guest state, the CS register is not + * accessible. The hypercall register values will have had to been + * provided in 64-bit mode, so assume the guest is in 64-bit. + */ + op_64_bit = is_64_bit_mode(vcpu) || vcpu->arch.guest_state_protected; if (!op_64_bit) { nr &= 0xFFFFFFFF; a0 &= 0xFFFFFFFF;
When processing a hypercall for a guest with protected state, currently SEV-ES guests, the guest CS segment register can't be checked to determine if the guest is in 64-bit mode. For an SEV-ES guest, it is expected that communication between the guest and the hypervisor is performed to shared memory using the GHCB. In order to use the GHCB, the guest must have been in long mode, otherwise writes by the guest to the GHCB would be encrypted and not be able to be comprehended by the hypervisor. Given that, assume that the guest is in 64-bit mode when processing a hypercall from a guest with protected state. Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES") Reported-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- arch/x86/kvm/x86.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)