[RFC,08/35] KVM: SVM: Prevent debugging under SEV-ES

Commit Message

Tom Lendacky Sept. 14, 2020, 8:15 p.m. UTC

From: Tom Lendacky <[e-mail]>

Since the guest register state of an SEV-ES guest is encrypted, debugging
is not supported. Update the code to prevent guest debugging when the
guest is an SEV-ES guest. This includes adding a callable function that
is used to determine if the guest supports being debugged.

Signed-off-by: Tom Lendacky <[e-mail]>
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/svm/svm.c          | 16 ++++++++++++++++
 arch/x86/kvm/vmx/vmx.c          |  7 +++++++
 arch/x86/kvm/x86.c              |  3 +++
 4 files changed, 28 insertions(+)

Comments

Sean Christopherson Sept. 14, 2020, 9:26 p.m. UTC | #1

On Mon, Sep 14, 2020 at 03:15:22PM -0500, Tom Lendacky wrote:
> From: Tom Lendacky <[e-mail]>
> 
> Since the guest register state of an SEV-ES guest is encrypted, debugging
> is not supported. Update the code to prevent guest debugging when the
> guest is an SEV-ES guest. This includes adding a callable function that
> is used to determine if the guest supports being debugged.
> 
> Signed-off-by: Tom Lendacky <[e-mail]>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/svm/svm.c          | 16 ++++++++++++++++
>  arch/x86/kvm/vmx/vmx.c          |  7 +++++++
>  arch/x86/kvm/x86.c              |  3 +++
>  4 files changed, 28 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c900992701d6..3e2a3d2a8ba8 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1234,6 +1234,8 @@ struct kvm_x86_ops {
>  	void (*reg_read_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
>  	void (*reg_write_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg,
>  				   unsigned long val);
> +
> +	bool (*allow_debug)(struct kvm *kvm);

Why add both allow_debug() and vmsa_encrypted?  I assume there are scenarios
where allow_debug() != vmsa_encrypted?  E.g. is there a debug mode for SEV-ES
where the VMSA is not encrypted, but KVM (ironically) can't intercept #DBs or
something?

Alternatively, have you explored using a new VM_TYPE for SEV-ES guests?  With
a genericized vmsa_encrypted, that would allow something like the following
for scenarios where the VMSA is not (yet?) encrypted for an SEV-ES guest.  I
don't love bleeding the VM type into x86.c, but for one-off quirks like this
I think it'd be preferable to adding a kvm_x86_ops hook.

int kvm_arch_vcpu_ioctl_set_guest_debug(...)
{
	if (vcpu->arch.guest_state_protected ||
	    kvm->arch.vm_type == KVM_X86_SEV_ES_VM)
		return -EINVAL;
}

Tom Lendacky Sept. 15, 2020, 1:37 p.m. UTC | #2

On 9/14/20 4:26 PM, Sean Christopherson wrote:
> On Mon, Sep 14, 2020 at 03:15:22PM -0500, Tom Lendacky wrote:
>> From: Tom Lendacky <[e-mail]>
>>
>> Since the guest register state of an SEV-ES guest is encrypted, debugging
>> is not supported. Update the code to prevent guest debugging when the
>> guest is an SEV-ES guest. This includes adding a callable function that
>> is used to determine if the guest supports being debugged.
>>
>> Signed-off-by: Tom Lendacky <[e-mail]>
>> ---
>>  arch/x86/include/asm/kvm_host.h |  2 ++
>>  arch/x86/kvm/svm/svm.c          | 16 ++++++++++++++++
>>  arch/x86/kvm/vmx/vmx.c          |  7 +++++++
>>  arch/x86/kvm/x86.c              |  3 +++
>>  4 files changed, 28 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index c900992701d6..3e2a3d2a8ba8 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1234,6 +1234,8 @@ struct kvm_x86_ops {
>>  	void (*reg_read_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
>>  	void (*reg_write_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg,
>>  				   unsigned long val);
>> +
>> +	bool (*allow_debug)(struct kvm *kvm);
> 
> Why add both allow_debug() and vmsa_encrypted?  I assume there are scenarios
> where allow_debug() != vmsa_encrypted?  E.g. is there a debug mode for SEV-ES
> where the VMSA is not encrypted, but KVM (ironically) can't intercept #DBs or
> something?

No, once the guest has had LAUNCH_UPDATE_VMSA run against the vCPUs, then
the vCPU states are all encrypted. But that doesn't mean that debugging
can't be done in the future.

> 
> Alternatively, have you explored using a new VM_TYPE for SEV-ES guests?  With
> a genericized vmsa_encrypted, that would allow something like the following
> for scenarios where the VMSA is not (yet?) encrypted for an SEV-ES guest.  I
> don't love bleeding the VM type into x86.c, but for one-off quirks like this
> I think it'd be preferable to adding a kvm_x86_ops hook.
> 
> int kvm_arch_vcpu_ioctl_set_guest_debug(...)
> {
> 	if (vcpu->arch.guest_state_protected ||
> 	    kvm->arch.vm_type == KVM_X86_SEV_ES_VM)
> 		return -EINVAL;
> }
> 

I haven't explored that, I'll look into it.

Thanks,
Tom

Sean Christopherson Sept. 15, 2020, 4:30 p.m. UTC | #3

On Tue, Sep 15, 2020 at 08:37:12AM -0500, Tom Lendacky wrote:
> On 9/14/20 4:26 PM, Sean Christopherson wrote:
> > On Mon, Sep 14, 2020 at 03:15:22PM -0500, Tom Lendacky wrote:
> >> From: Tom Lendacky <[e-mail]>
> >>
> >> Since the guest register state of an SEV-ES guest is encrypted, debugging
> >> is not supported. Update the code to prevent guest debugging when the
> >> guest is an SEV-ES guest. This includes adding a callable function that
> >> is used to determine if the guest supports being debugged.
> >>
> >> Signed-off-by: Tom Lendacky <[e-mail]>
> >> ---
> >>  arch/x86/include/asm/kvm_host.h |  2 ++
> >>  arch/x86/kvm/svm/svm.c          | 16 ++++++++++++++++
> >>  arch/x86/kvm/vmx/vmx.c          |  7 +++++++
> >>  arch/x86/kvm/x86.c              |  3 +++
> >>  4 files changed, 28 insertions(+)
> >>
> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >> index c900992701d6..3e2a3d2a8ba8 100644
> >> --- a/arch/x86/include/asm/kvm_host.h
> >> +++ b/arch/x86/include/asm/kvm_host.h
> >> @@ -1234,6 +1234,8 @@ struct kvm_x86_ops {
> >>  	void (*reg_read_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
> >>  	void (*reg_write_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg,
> >>  				   unsigned long val);
> >> +
> >> +	bool (*allow_debug)(struct kvm *kvm);
> > 
> > Why add both allow_debug() and vmsa_encrypted?  I assume there are scenarios
> > where allow_debug() != vmsa_encrypted?  E.g. is there a debug mode for SEV-ES
> > where the VMSA is not encrypted, but KVM (ironically) can't intercept #DBs or
> > something?
> 
> No, once the guest has had LAUNCH_UPDATE_VMSA run against the vCPUs, then
> the vCPU states are all encrypted. But that doesn't mean that debugging
> can't be done in the future.

I don't quite follow the "doesn't mean debugging can't be done in the future".
Does that imply that debugging could be supported for SEV-ES guests, even if
they have an encrypted VMSA?

Tom Lendacky Sept. 15, 2020, 8:13 p.m. UTC | #4

On 9/15/20 11:30 AM, Sean Christopherson wrote:
> On Tue, Sep 15, 2020 at 08:37:12AM -0500, Tom Lendacky wrote:
>> On 9/14/20 4:26 PM, Sean Christopherson wrote:
>>> On Mon, Sep 14, 2020 at 03:15:22PM -0500, Tom Lendacky wrote:
>>>> From: Tom Lendacky <[e-mail]>
>>>>
>>>> Since the guest register state of an SEV-ES guest is encrypted, debugging
>>>> is not supported. Update the code to prevent guest debugging when the
>>>> guest is an SEV-ES guest. This includes adding a callable function that
>>>> is used to determine if the guest supports being debugged.
>>>>
>>>> Signed-off-by: Tom Lendacky <[e-mail]>
>>>> ---
>>>>  arch/x86/include/asm/kvm_host.h |  2 ++
>>>>  arch/x86/kvm/svm/svm.c          | 16 ++++++++++++++++
>>>>  arch/x86/kvm/vmx/vmx.c          |  7 +++++++
>>>>  arch/x86/kvm/x86.c              |  3 +++
>>>>  4 files changed, 28 insertions(+)
>>>>
>>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>>> index c900992701d6..3e2a3d2a8ba8 100644
>>>> --- a/arch/x86/include/asm/kvm_host.h
>>>> +++ b/arch/x86/include/asm/kvm_host.h
>>>> @@ -1234,6 +1234,8 @@ struct kvm_x86_ops {
>>>>  	void (*reg_read_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
>>>>  	void (*reg_write_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg,
>>>>  				   unsigned long val);
>>>> +
>>>> +	bool (*allow_debug)(struct kvm *kvm);
>>>
>>> Why add both allow_debug() and vmsa_encrypted?  I assume there are scenarios
>>> where allow_debug() != vmsa_encrypted?  E.g. is there a debug mode for SEV-ES
>>> where the VMSA is not encrypted, but KVM (ironically) can't intercept #DBs or
>>> something?
>>
>> No, once the guest has had LAUNCH_UPDATE_VMSA run against the vCPUs, then
>> the vCPU states are all encrypted. But that doesn't mean that debugging
>> can't be done in the future.
> 
> I don't quite follow the "doesn't mean debugging can't be done in the future".
> Does that imply that debugging could be supported for SEV-ES guests, even if
> they have an encrypted VMSA?

Almost anything can be done with software. It would require a lot of
hypervisor and guest code and changes to the GHCB spec, etc. So given
that, probably just the check for arch.guest_state_protected is enough for
now. I'll just need to be sure none of the debugging paths can be taken
before the VMSA is encrypted.

Thanks,
Tom

>

Tom Lendacky Sept. 16, 2020, 3:11 p.m. UTC | #5

On 9/15/20 3:13 PM, Tom Lendacky wrote:
> On 9/15/20 11:30 AM, Sean Christopherson wrote:
>> On Tue, Sep 15, 2020 at 08:37:12AM -0500, Tom Lendacky wrote:
>>> On 9/14/20 4:26 PM, Sean Christopherson wrote:
>>>> On Mon, Sep 14, 2020 at 03:15:22PM -0500, Tom Lendacky wrote:
>>>>> From: Tom Lendacky <[e-mail]>
>>>>>
>>>>> Since the guest register state of an SEV-ES guest is encrypted, debugging
>>>>> is not supported. Update the code to prevent guest debugging when the
>>>>> guest is an SEV-ES guest. This includes adding a callable function that
>>>>> is used to determine if the guest supports being debugged.
>>>>>
>>>>> Signed-off-by: Tom Lendacky <[e-mail]>
>>>>> ---
>>>>>  arch/x86/include/asm/kvm_host.h |  2 ++
>>>>>  arch/x86/kvm/svm/svm.c          | 16 ++++++++++++++++
>>>>>  arch/x86/kvm/vmx/vmx.c          |  7 +++++++
>>>>>  arch/x86/kvm/x86.c              |  3 +++
>>>>>  4 files changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>>>> index c900992701d6..3e2a3d2a8ba8 100644
>>>>> --- a/arch/x86/include/asm/kvm_host.h
>>>>> +++ b/arch/x86/include/asm/kvm_host.h
>>>>> @@ -1234,6 +1234,8 @@ struct kvm_x86_ops {
>>>>>  	void (*reg_read_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
>>>>>  	void (*reg_write_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg,
>>>>>  				   unsigned long val);
>>>>> +
>>>>> +	bool (*allow_debug)(struct kvm *kvm);
>>>>
>>>> Why add both allow_debug() and vmsa_encrypted?  I assume there are scenarios
>>>> where allow_debug() != vmsa_encrypted?  E.g. is there a debug mode for SEV-ES
>>>> where the VMSA is not encrypted, but KVM (ironically) can't intercept #DBs or
>>>> something?
>>>
>>> No, once the guest has had LAUNCH_UPDATE_VMSA run against the vCPUs, then
>>> the vCPU states are all encrypted. But that doesn't mean that debugging
>>> can't be done in the future.
>>
>> I don't quite follow the "doesn't mean debugging can't be done in the future".
>> Does that imply that debugging could be supported for SEV-ES guests, even if
>> they have an encrypted VMSA?
> 
> Almost anything can be done with software. It would require a lot of
> hypervisor and guest code and changes to the GHCB spec, etc. So given
> that, probably just the check for arch.guest_state_protected is enough for
> now. I'll just need to be sure none of the debugging paths can be taken
> before the VMSA is encrypted.

So I don't think there's any guarantee that the KVM_SET_GUEST_DEBUG ioctl
couldn't be called before the VMSA is encrypted, meaning I can't check the
arch.guest_state_protected bit for that call. So if we really want to get
rid of the allow_debug() op, I'd need some other way to indicate that this
is an SEV-ES / protected state guest.

How are you planning on blocking this ioctl for TDX? Would the
arch.guest_state_protected bit be sit earlier than is done for SEV-ES?

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>

Sean Christopherson Sept. 16, 2020, 4:02 p.m. UTC | #6

On Wed, Sep 16, 2020 at 10:11:10AM -0500, Tom Lendacky wrote:
> On 9/15/20 3:13 PM, Tom Lendacky wrote:
> > On 9/15/20 11:30 AM, Sean Christopherson wrote:
> >> I don't quite follow the "doesn't mean debugging can't be done in the future".
> >> Does that imply that debugging could be supported for SEV-ES guests, even if
> >> they have an encrypted VMSA?
> > 
> > Almost anything can be done with software. It would require a lot of
> > hypervisor and guest code and changes to the GHCB spec, etc. So given
> > that, probably just the check for arch.guest_state_protected is enough for
> > now. I'll just need to be sure none of the debugging paths can be taken
> > before the VMSA is encrypted.
> 
> So I don't think there's any guarantee that the KVM_SET_GUEST_DEBUG ioctl
> couldn't be called before the VMSA is encrypted, meaning I can't check the
> arch.guest_state_protected bit for that call. So if we really want to get
> rid of the allow_debug() op, I'd need some other way to indicate that this
> is an SEV-ES / protected state guest.

Would anything break if KVM "speculatively" set guest_state_protected before
LAUNCH_UPDATE_VMSA?  E.g. does KVM need to emulate before LAUNCH_UPDATE_VMSA?

> How are you planning on blocking this ioctl for TDX? Would the
> arch.guest_state_protected bit be sit earlier than is done for SEV-ES?

Yep, guest_state_protected is set from time zero (kvm_x86_ops.vm_init) as
guest state is encrypted/inaccessible from the get go.  The flag actually
gets turned off for debuggable TDX guests, but that's also forced to happen
before the KVM_RUN can be invoked (TDX architecture) and is a one-time
configuration, i.e. userspace can flip the switch exactly once, and only at
a very specific point in time.

Tom Lendacky Sept. 16, 2020, 4:38 p.m. UTC | #7

On 9/16/20 11:02 AM, Sean Christopherson wrote:
> On Wed, Sep 16, 2020 at 10:11:10AM -0500, Tom Lendacky wrote:
>> On 9/15/20 3:13 PM, Tom Lendacky wrote:
>>> On 9/15/20 11:30 AM, Sean Christopherson wrote:
>>>> I don't quite follow the "doesn't mean debugging can't be done in the future".
>>>> Does that imply that debugging could be supported for SEV-ES guests, even if
>>>> they have an encrypted VMSA?
>>>
>>> Almost anything can be done with software. It would require a lot of
>>> hypervisor and guest code and changes to the GHCB spec, etc. So given
>>> that, probably just the check for arch.guest_state_protected is enough for
>>> now. I'll just need to be sure none of the debugging paths can be taken
>>> before the VMSA is encrypted.
>>
>> So I don't think there's any guarantee that the KVM_SET_GUEST_DEBUG ioctl
>> couldn't be called before the VMSA is encrypted, meaning I can't check the
>> arch.guest_state_protected bit for that call. So if we really want to get
>> rid of the allow_debug() op, I'd need some other way to indicate that this
>> is an SEV-ES / protected state guest.
> 
> Would anything break if KVM "speculatively" set guest_state_protected before
> LAUNCH_UPDATE_VMSA?  E.g. does KVM need to emulate before LAUNCH_UPDATE_VMSA?

Yes, the way the code is set up, the guest state (VMSA) is initialized in
the same way it is today (mostly) and that state is encrypted by the
LAUNCH_UPDATE_VMSA call. I check the guest_state_protected bit to decide
on whether to direct the updates to the real VMSA (before it's encrypted)
or the GHCB (that's the get_vmsa() function from patch #5).

Thanks,
Tom

> 
>> How are you planning on blocking this ioctl for TDX? Would the
>> arch.guest_state_protected bit be sit earlier than is done for SEV-ES?
> 
> Yep, guest_state_protected is set from time zero (kvm_x86_ops.vm_init) as
> guest state is encrypted/inaccessible from the get go.  The flag actually
> gets turned off for debuggable TDX guests, but that's also forced to happen
> before the KVM_RUN can be invoked (TDX architecture) and is a one-time
> configuration, i.e. userspace can flip the switch exactly once, and only at
> a very specific point in time.
>

Sean Christopherson Sept. 16, 2020, 4:49 p.m. UTC | #8

On Wed, Sep 16, 2020 at 11:38:38AM -0500, Tom Lendacky wrote:
> 
> 
> On 9/16/20 11:02 AM, Sean Christopherson wrote:
> > On Wed, Sep 16, 2020 at 10:11:10AM -0500, Tom Lendacky wrote:
> >> On 9/15/20 3:13 PM, Tom Lendacky wrote:
> >>> On 9/15/20 11:30 AM, Sean Christopherson wrote:
> >>>> I don't quite follow the "doesn't mean debugging can't be done in the future".
> >>>> Does that imply that debugging could be supported for SEV-ES guests, even if
> >>>> they have an encrypted VMSA?
> >>>
> >>> Almost anything can be done with software. It would require a lot of
> >>> hypervisor and guest code and changes to the GHCB spec, etc. So given
> >>> that, probably just the check for arch.guest_state_protected is enough for
> >>> now. I'll just need to be sure none of the debugging paths can be taken
> >>> before the VMSA is encrypted.
> >>
> >> So I don't think there's any guarantee that the KVM_SET_GUEST_DEBUG ioctl
> >> couldn't be called before the VMSA is encrypted, meaning I can't check the
> >> arch.guest_state_protected bit for that call. So if we really want to get
> >> rid of the allow_debug() op, I'd need some other way to indicate that this
> >> is an SEV-ES / protected state guest.
> > 
> > Would anything break if KVM "speculatively" set guest_state_protected before
> > LAUNCH_UPDATE_VMSA?  E.g. does KVM need to emulate before LAUNCH_UPDATE_VMSA?
> 
> Yes, the way the code is set up, the guest state (VMSA) is initialized in
> the same way it is today (mostly) and that state is encrypted by the
> LAUNCH_UPDATE_VMSA call. I check the guest_state_protected bit to decide
> on whether to direct the updates to the real VMSA (before it's encrypted)
> or the GHCB (that's the get_vmsa() function from patch #5).

Ah, gotcha.  Would it work to set guest_state_protected[*] from time zero,
and move vmsa_encrypted to struct vcpu_svm?  I.e. keep vmsa_encrypted, but
use it only for guiding get_vmsa() and related behavior.

Tom Lendacky Sept. 16, 2020, 8:27 p.m. UTC | #9

On 9/16/20 11:49 AM, Sean Christopherson wrote:
> On Wed, Sep 16, 2020 at 11:38:38AM -0500, Tom Lendacky wrote:
>>
>>
>> On 9/16/20 11:02 AM, Sean Christopherson wrote:
>>> On Wed, Sep 16, 2020 at 10:11:10AM -0500, Tom Lendacky wrote:
>>>> On 9/15/20 3:13 PM, Tom Lendacky wrote:
>>>>> On 9/15/20 11:30 AM, Sean Christopherson wrote:
>>>>>> I don't quite follow the "doesn't mean debugging can't be done in the future".
>>>>>> Does that imply that debugging could be supported for SEV-ES guests, even if
>>>>>> they have an encrypted VMSA?
>>>>>
>>>>> Almost anything can be done with software. It would require a lot of
>>>>> hypervisor and guest code and changes to the GHCB spec, etc. So given
>>>>> that, probably just the check for arch.guest_state_protected is enough for
>>>>> now. I'll just need to be sure none of the debugging paths can be taken
>>>>> before the VMSA is encrypted.
>>>>
>>>> So I don't think there's any guarantee that the KVM_SET_GUEST_DEBUG ioctl
>>>> couldn't be called before the VMSA is encrypted, meaning I can't check the
>>>> arch.guest_state_protected bit for that call. So if we really want to get
>>>> rid of the allow_debug() op, I'd need some other way to indicate that this
>>>> is an SEV-ES / protected state guest.
>>>
>>> Would anything break if KVM "speculatively" set guest_state_protected before
>>> LAUNCH_UPDATE_VMSA?  E.g. does KVM need to emulate before LAUNCH_UPDATE_VMSA?
>>
>> Yes, the way the code is set up, the guest state (VMSA) is initialized in
>> the same way it is today (mostly) and that state is encrypted by the
>> LAUNCH_UPDATE_VMSA call. I check the guest_state_protected bit to decide
>> on whether to direct the updates to the real VMSA (before it's encrypted)
>> or the GHCB (that's the get_vmsa() function from patch #5).
> 
> Ah, gotcha.  Would it work to set guest_state_protected[*] from time zero,
> and move vmsa_encrypted to struct vcpu_svm?  I.e. keep vmsa_encrypted, but
> use it only for guiding get_vmsa() and related behavior.

It is mainly __set_sregs() that needs to know when to allow the register
writes and when not to. During guest initialization, __set_sregs is how
some of the VMSA is initialized by Qemu.

Thanks,
Tom

>

Sean Christopherson Sept. 16, 2020, 10:50 p.m. UTC | #10

On Wed, Sep 16, 2020 at 03:27:13PM -0500, Tom Lendacky wrote:
> On 9/16/20 11:49 AM, Sean Christopherson wrote:
> > On Wed, Sep 16, 2020 at 11:38:38AM -0500, Tom Lendacky wrote:
> >>
> >>
> >> On 9/16/20 11:02 AM, Sean Christopherson wrote:
> >>> On Wed, Sep 16, 2020 at 10:11:10AM -0500, Tom Lendacky wrote:
> >>>> On 9/15/20 3:13 PM, Tom Lendacky wrote:
> >>>>> On 9/15/20 11:30 AM, Sean Christopherson wrote:
> >>>>>> I don't quite follow the "doesn't mean debugging can't be done in the future".
> >>>>>> Does that imply that debugging could be supported for SEV-ES guests, even if
> >>>>>> they have an encrypted VMSA?
> >>>>>
> >>>>> Almost anything can be done with software. It would require a lot of
> >>>>> hypervisor and guest code and changes to the GHCB spec, etc. So given
> >>>>> that, probably just the check for arch.guest_state_protected is enough for
> >>>>> now. I'll just need to be sure none of the debugging paths can be taken
> >>>>> before the VMSA is encrypted.
> >>>>
> >>>> So I don't think there's any guarantee that the KVM_SET_GUEST_DEBUG ioctl
> >>>> couldn't be called before the VMSA is encrypted, meaning I can't check the
> >>>> arch.guest_state_protected bit for that call. So if we really want to get
> >>>> rid of the allow_debug() op, I'd need some other way to indicate that this
> >>>> is an SEV-ES / protected state guest.
> >>>
> >>> Would anything break if KVM "speculatively" set guest_state_protected before
> >>> LAUNCH_UPDATE_VMSA?  E.g. does KVM need to emulate before LAUNCH_UPDATE_VMSA?
> >>
> >> Yes, the way the code is set up, the guest state (VMSA) is initialized in
> >> the same way it is today (mostly) and that state is encrypted by the
> >> LAUNCH_UPDATE_VMSA call. I check the guest_state_protected bit to decide
> >> on whether to direct the updates to the real VMSA (before it's encrypted)
> >> or the GHCB (that's the get_vmsa() function from patch #5).
> > 
> > Ah, gotcha.  Would it work to set guest_state_protected[*] from time zero,
> > and move vmsa_encrypted to struct vcpu_svm?  I.e. keep vmsa_encrypted, but
> > use it only for guiding get_vmsa() and related behavior.
> 
> It is mainly __set_sregs() that needs to know when to allow the register
> writes and when not to. During guest initialization, __set_sregs is how
> some of the VMSA is initialized by Qemu.

Hmm.  I assume that also means KVM_SET_REGS and KVM_GET_XCRS are also legal
before the VMSA is encrypted?  If so, then the current behavior of setting
vmsa_encrypted "late" make sense.  KVM_SET_FPU/XSAVE can be handled by not
allocating guest_fpu, i.e. they can be disallowed from time zero without
adding an SEV-ES specific check.

Which brings us back to KVM_SET_GUEST_DEBUG.  What would happen if that were
allowed prior to VMSA encryption?  If LAUNCH_UPDATE_VMSA acts as a sort of
reset, one thought would be to allow KVM_SET_GUEST_DEBUG and then sanitize
KVM's state during LAUNCH_UPDATE_VMSA.  Or perhaps even better, disallow
LAUNCH_UPDATE_VMSA if vcpu->guest_debug!=0.  That would allow using debug
capabilities up until LAUNCH_UPDATE_VMSA without adding much burden to KVM.

Tom Lendacky Sept. 17, 2020, 4:27 p.m. UTC | #11

On 9/16/20 5:50 PM, Sean Christopherson wrote:
> On Wed, Sep 16, 2020 at 03:27:13PM -0500, Tom Lendacky wrote:
>> On 9/16/20 11:49 AM, Sean Christopherson wrote:
>>> On Wed, Sep 16, 2020 at 11:38:38AM -0500, Tom Lendacky wrote:
>>>>
>>>>
>>>> On 9/16/20 11:02 AM, Sean Christopherson wrote:
>>>>> On Wed, Sep 16, 2020 at 10:11:10AM -0500, Tom Lendacky wrote:
>>>>>> On 9/15/20 3:13 PM, Tom Lendacky wrote:
>>>>>>> On 9/15/20 11:30 AM, Sean Christopherson wrote:
>>>>>>>> I don't quite follow the "doesn't mean debugging can't be done in the future".
>>>>>>>> Does that imply that debugging could be supported for SEV-ES guests, even if
>>>>>>>> they have an encrypted VMSA?
>>>>>>>
>>>>>>> Almost anything can be done with software. It would require a lot of
>>>>>>> hypervisor and guest code and changes to the GHCB spec, etc. So given
>>>>>>> that, probably just the check for arch.guest_state_protected is enough for
>>>>>>> now. I'll just need to be sure none of the debugging paths can be taken
>>>>>>> before the VMSA is encrypted.
>>>>>>
>>>>>> So I don't think there's any guarantee that the KVM_SET_GUEST_DEBUG ioctl
>>>>>> couldn't be called before the VMSA is encrypted, meaning I can't check the
>>>>>> arch.guest_state_protected bit for that call. So if we really want to get
>>>>>> rid of the allow_debug() op, I'd need some other way to indicate that this
>>>>>> is an SEV-ES / protected state guest.
>>>>>
>>>>> Would anything break if KVM "speculatively" set guest_state_protected before
>>>>> LAUNCH_UPDATE_VMSA?  E.g. does KVM need to emulate before LAUNCH_UPDATE_VMSA?
>>>>
>>>> Yes, the way the code is set up, the guest state (VMSA) is initialized in
>>>> the same way it is today (mostly) and that state is encrypted by the
>>>> LAUNCH_UPDATE_VMSA call. I check the guest_state_protected bit to decide
>>>> on whether to direct the updates to the real VMSA (before it's encrypted)
>>>> or the GHCB (that's the get_vmsa() function from patch #5).
>>>
>>> Ah, gotcha.  Would it work to set guest_state_protected[*] from time zero,
>>> and move vmsa_encrypted to struct vcpu_svm?  I.e. keep vmsa_encrypted, but
>>> use it only for guiding get_vmsa() and related behavior.
>>
>> It is mainly __set_sregs() that needs to know when to allow the register
>> writes and when not to. During guest initialization, __set_sregs is how
>> some of the VMSA is initialized by Qemu.
> 
> Hmm.  I assume that also means KVM_SET_REGS and KVM_GET_XCRS are also legal
> before the VMSA is encrypted?  If so, then the current behavior of setting
> vmsa_encrypted "late" make sense.  KVM_SET_FPU/XSAVE can be handled by not
> allocating guest_fpu, i.e. they can be disallowed from time zero without
> adding an SEV-ES specific check.
> 
> Which brings us back to KVM_SET_GUEST_DEBUG.  What would happen if that were
> allowed prior to VMSA encryption?  If LAUNCH_UPDATE_VMSA acts as a sort of
> reset, one thought would be to allow KVM_SET_GUEST_DEBUG and then sanitize
> KVM's state during LAUNCH_UPDATE_VMSA.  Or perhaps even better, disallow
> LAUNCH_UPDATE_VMSA if vcpu->guest_debug!=0.  That would allow using debug
> capabilities up until LAUNCH_UPDATE_VMSA without adding much burden to KVM.

I think the vcpu->guest_debug check before the LAUNCH_UPDATE_VMSA would be 
good. I'll remove the allow_debug() op and replace it with the 
guest_state_protected check in its place.

Thanks,
Tom

>

Patch

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c900992701d6..3e2a3d2a8ba8 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1234,6 +1234,8 @@  struct kvm_x86_ops {
 	void (*reg_read_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	void (*reg_write_override)(struct kvm_vcpu *vcpu, enum kvm_reg reg,
 				   unsigned long val);
+
+	bool (*allow_debug)(struct kvm *kvm);
 };
 
 struct kvm_x86_nested_ops {
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index f8a5b7164008..47fa2067609a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1729,6 +1729,9 @@  static void svm_set_dr6(struct vcpu_svm *svm, unsigned long value)
 {
 	struct vmcb *vmcb = svm->vmcb;
 
+	if (svm->vcpu.arch.vmsa_encrypted)
+		return;
+
 	if (unlikely(value != svm_dr6_read(svm))) {
 		svm_dr6_write(svm, value);
 		vmcb_mark_dirty(vmcb, VMCB_DR);
@@ -1739,6 +1742,9 @@  static void svm_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (vcpu->arch.vmsa_encrypted)
+		return;
+
 	get_debugreg(vcpu->arch.db[0], 0);
 	get_debugreg(vcpu->arch.db[1], 1);
 	get_debugreg(vcpu->arch.db[2], 2);
@@ -1757,6 +1763,9 @@  static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (vcpu->arch.vmsa_encrypted)
+		return;
+
 	svm_dr7_write(svm, value);
 	vmcb_mark_dirty(svm->vmcb, VMCB_DR);
 }
@@ -4243,6 +4252,11 @@  static void svm_reg_write_override(struct kvm_vcpu *vcpu, enum kvm_reg reg,
 	vmsa_reg[entry] = val;
 }
 
+static bool svm_allow_debug(struct kvm *kvm)
+{
+	return !sev_es_guest(kvm);
+}
+
 static void svm_vm_destroy(struct kvm *kvm)
 {
 	avic_vm_destroy(kvm);
@@ -4384,6 +4398,8 @@  static struct kvm_x86_ops svm_x86_ops __initdata = {
 
 	.reg_read_override = svm_reg_read_override,
 	.reg_write_override = svm_reg_write_override,
+
+	.allow_debug = svm_allow_debug,
 };
 
 static struct kvm_x86_init_ops svm_init_ops __initdata = {
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 46ba2e03a892..fb8591bba96f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7879,6 +7879,11 @@  static bool vmx_check_apicv_inhibit_reasons(ulong bit)
 	return supported & BIT(bit);
 }
 
+static bool vmx_allow_debug(struct kvm *kvm)
+{
+	return true;
+}
+
 static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.hardware_unsetup = hardware_unsetup,
 
@@ -8005,6 +8010,8 @@  static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.need_emulation_on_page_fault = vmx_need_emulation_on_page_fault,
 	.apic_init_signal_blocked = vmx_apic_init_signal_blocked,
 	.migrate_timers = vmx_migrate_timers,
+
+	.allow_debug = vmx_allow_debug,
 };
 
 static __init int hardware_setup(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a5afdccb6c17..9970c0b7854f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9279,6 +9279,9 @@  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 	unsigned long rflags;
 	int i, r;
 
+	if (!kvm_x86_ops.allow_debug(vcpu->kvm))
+		return -EINVAL;
+
 	vcpu_load(vcpu);
 
 	if (dbg->control & (KVM_GUESTDBG_INJECT_DB | KVM_GUESTDBG_INJECT_BP)) {