Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Tom Lendacky]

On 10/1/21 12:06 PM, Tom Lendacky wrote:
> On 5/25/21 1:25 AM, Vitaly Kuznetsov wrote:
> > Tom Lendacky <thomas.lendacky@xxxxxxx> 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.
> > >
> > > Create a new helper function, is_64_bit_hypercall(), that assumes the
> > > guest is in 64-bit mode when the guest has protected state, and returns
> > > true, otherwise invoking is_64_bit_mode() to determine the mode. Update
> > > the hypercall related routines to use is_64_bit_hypercall() instead of
> > > is_64_bit_mode().
> > >
> > > Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
> > > this helper function for a guest running with protected state.
> > >
> > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support 
> > > intercepts under SEV-ES")
> > > Reported-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > > ---
> > >
> > > Changes since v1:
> > > - Create a new helper routine, is_64_bit_hypercall(), and use it in place
> > >    of is_64_bit_mode() in hypercall related areas.
> > > - Add a WARN_ON_ONCE() to is_64_bit_mode() to issue a warning if invoked
> > >    for a guest with protected state.
> > > ---
> > >   arch/x86/kvm/hyperv.c |  4 ++--
> > >   arch/x86/kvm/x86.c    |  2 +-
> > >   arch/x86/kvm/x86.h    | 12 ++++++++++++
> > >   arch/x86/kvm/xen.c    |  2 +-
> > >   4 files changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > > index f98370a39936..1cdf2b213f41 100644
> > > --- a/arch/x86/kvm/hyperv.c
> > > +++ b/arch/x86/kvm/hyperv.c
> > > @@ -1818,7 +1818,7 @@ static void kvm_hv_hypercall_set_result(struct 
> > > kvm_vcpu *vcpu, u64 result)
> > >   {
> > >       bool longmode;
> > > -    longmode = is_64_bit_mode(vcpu);
> > > +    longmode = is_64_bit_hypercall(vcpu);
> > >       if (longmode)
> > >           kvm_rax_write(vcpu, result);
> > >       else {
> > > @@ -1895,7 +1895,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> > >       }
> > >   #ifdef CONFIG_X86_64
> > > -    if (is_64_bit_mode(vcpu)) {
> > > +    if (is_64_bit_hypercall(vcpu)) {
> > >           param = kvm_rcx_read(vcpu);
> > >           ingpa = kvm_rdx_read(vcpu);
> > >           outgpa = kvm_r8_read(vcpu);
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 9b6bca616929..dc72f0a1609a 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -8403,7 +8403,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > >       trace_kvm_hypercall(nr, a0, a1, a2, a3);
> > > -    op_64_bit = is_64_bit_mode(vcpu);
> > > +    op_64_bit = is_64_bit_hypercall(vcpu);
> > >       if (!op_64_bit) {
> > >           nr &= 0xFFFFFFFF;
> > >           a0 &= 0xFFFFFFFF;
> > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > > index 521f74e5bbf2..3102caf689d2 100644
> > > --- a/arch/x86/kvm/x86.h
> > > +++ b/arch/x86/kvm/x86.h
> > > @@ -151,12 +151,24 @@ static inline bool is_64_bit_mode(struct kvm_vcpu 
> > > *vcpu)
> > >   {
> > >       int cs_db, cs_l;
> > > +    WARN_ON_ONCE(vcpu->arch.guest_state_protected);
> > > +
> > >       if (!is_long_mode(vcpu))
> > >           return false;
> > >       static_call(kvm_x86_get_cs_db_l_bits)(vcpu, &cs_db, &cs_l);
> > >       return cs_l;
> > >   }
> > > +static inline bool is_64_bit_hypercall(struct kvm_vcpu *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.
> > > +     */
> > > +    return vcpu->arch.guest_state_protected || is_64_bit_mode(vcpu);
> > > +}
> > > +
> > >   static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
> > >   {
> > >   #ifdef CONFIG_X86_64
> > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > > index ae17250e1efe..c58f6369e668 100644
> > > --- a/arch/x86/kvm/xen.c
> > > +++ b/arch/x86/kvm/xen.c
> > > @@ -680,7 +680,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
> > >           kvm_hv_hypercall_enabled(vcpu))
> > >           return kvm_hv_hypercall(vcpu);
> > > -    longmode = is_64_bit_mode(vcpu);
> > > +    longmode = is_64_bit_hypercall(vcpu);
> > >       if (!longmode) {
> > >           params[0] = (u32)kvm_rbx_read(vcpu);
> > >           params[1] = (u32)kvm_rcx_read(vcpu);
> >
> > Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> >
> > Thanks!
>
> Paolo,
>
> This got lost in my stack of work... any comments?
>
> Thanks,
> Tom

Ping

Thanks,
Tom

> >