[PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

When the host is using debug registers but the guest is not using them
nor is the guest in guest-debug state, the kvm code does not reset
the host debug registers before kvm_x86->run().  Rather, it relies on
the hardware vmentry instruction to automatically reset the dr7 registers
which ensures that the host breakpoints do not affect the guest.

But there are still problems:
	o The addresses of the host breakpoints can leak into the guest
	  and the guest may use these information to attack the host.

	o It violates the non-instrumentable nature around VM entry and
	  exit.  For example, when a host breakpoint is set on
	  vcpu->arch.cr2, #DB will hit aftr kvm_guest_enter_irqoff().

Beside the problems, the logic is not consistent either. When the guest
debug registers are active, the host breakpoints are reset before
kvm_x86->run(). But when the guest debug registers are inactive, the
host breakpoints are delayed to be disabled.  The host tracing tools may
see different results depending on there is any guest running or not.

To fix the problems, we also reload the debug registers before
kvm_x86->run() when the host is using them whenever the guest is using
them or not.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b594275d49b5..cce316655d3c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9320,7 +9320,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
 		switch_fpu_return();
 
-	if (unlikely(vcpu->arch.switch_db_regs)) {
+	if (unlikely(vcpu->arch.switch_db_regs || hw_breakpoint_active())) {
 		set_debugreg(0, 7);
 		set_debugreg(vcpu->arch.eff_db[0], 0);
 		set_debugreg(vcpu->arch.eff_db[1], 1);
-- 
2.19.1.6.gb485710b

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Paolo Bonzini]

On 28/06/21 19:26, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> When the host is using debug registers but the guest is not using them
> nor is the guest in guest-debug state, the kvm code does not reset
> the host debug registers before kvm_x86->run().  Rather, it relies on
> the hardware vmentry instruction to automatically reset the dr7 registers
> which ensures that the host breakpoints do not affect the guest.
> 
> But there are still problems:
> 	o The addresses of the host breakpoints can leak into the guest
> 	  and the guest may use these information to attack the host.

I don't think this is true, because DRn reads would exit (if they don't, 
switch_db_regs would be nonzero).  But otherwise it makes sense to do at 
least the DR7 write, and we might as well do all of them.

> 	o It violates the non-instrumentable nature around VM entry and
> 	  exit.  For example, when a host breakpoint is set on
> 	  vcpu->arch.cr2, #DB will hit aftr kvm_guest_enter_irqoff().
> 
> Beside the problems, the logic is not consistent either. When the guest
> debug registers are active, the host breakpoints are reset before
> kvm_x86->run(). But when the guest debug registers are inactive, the
> host breakpoints are delayed to be disabled.  The host tracing tools may
> see different results depending on there is any guest running or not.

More precisely, the host tracing tools may see different results 
depending on what the guest is doing.

Queued (with fixed commit message), thanks!

Paolo

> To fix the problems, we also reload the debug registers before
> kvm_x86->run() when the host is using them whenever the guest is using
> them or not.
> 
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>   arch/x86/kvm/x86.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b594275d49b5..cce316655d3c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9320,7 +9320,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>   	if (test_thread_flag(TIF_NEED_FPU_LOAD))
>   		switch_fpu_return();
>   
> -	if (unlikely(vcpu->arch.switch_db_regs)) {
> +	if (unlikely(vcpu->arch.switch_db_regs || hw_breakpoint_active())) {
>   		set_debugreg(0, 7);
>   		set_debugreg(vcpu->arch.eff_db[0], 0);
>   		set_debugreg(vcpu->arch.eff_db[1], 1);
> 

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Lai Jiangshan]

On 2021/7/9 00:48, Paolo Bonzini wrote:
> On 28/06/21 19:26, Lai Jiangshan wrote:
>> From: Lai Jiangshan <laijs@linux.alibaba.com>
>>
>> When the host is using debug registers but the guest is not using them
>> nor is the guest in guest-debug state, the kvm code does not reset
>> the host debug registers before kvm_x86->run().  Rather, it relies on
>> the hardware vmentry instruction to automatically reset the dr7 registers
>> which ensures that the host breakpoints do not affect the guest.
>>
>> But there are still problems:
>>     o The addresses of the host breakpoints can leak into the guest
>>       and the guest may use these information to attack the host.
> 
> I don't think this is true, because DRn reads would exit (if they don't, switch_db_regs would be nonzero).  But 
> otherwise it makes sense to do at least the DR7 write, and we might as well do all of them.

Ahh.... you are right.

> 
>>     o It violates the non-instrumentable nature around VM entry and
>>       exit.  For example, when a host breakpoint is set on
>>       vcpu->arch.cr2, #DB will hit aftr kvm_guest_enter_irqoff().
>>
>> Beside the problems, the logic is not consistent either. When the guest
>> debug registers are active, the host breakpoints are reset before
>> kvm_x86->run(). But when the guest debug registers are inactive, the
>> host breakpoints are delayed to be disabled.  The host tracing tools may
>> see different results depending on there is any guest running or not.
> 
> More precisely, the host tracing tools may see different results depending on what the guest is doing.
> 
> Queued (with fixed commit message), thanks!
> 
> Paolo

I just noticed that emulation.c fails to emulate with DBn.
Is there any problem around it?

For code breakpoint, if the instruction didn't cause vm-exit,
(for example, the 2nd instruction when kvm emulates instructions
back to back) emulation.c fails to emulate with DBn.

For code breakpoint, if the instruction just caused vm-exit.
It is difficult to analyze this case due to the complex priorities
between vectored events and fault-like vm-exit.
Anyway, if it is an instruction that vm-exit has priority over #DB,
emulation.c fails to emulate with DBn.

For data breakpoint, a #DB must be delivered to guest or to VMM (when
guest-debug) after the instruction. But emulation.c doesn't do so.

And the existence of both of effective DBn (guest debug) and guest DBn
complicates the problem when we try to emulate them.

Thanks.
Lai.

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Paolo Bonzini]

On 09/07/21 05:09, Lai Jiangshan wrote:
> I just noticed that emulation.c fails to emulate with DBn.
> Is there any problem around it?

Just what you said, it's not easy and the needs are limited.  I 
implemented kvm_vcpu_check_breakpoint because I was interested in using 
hardware breakpoints from gdb, even with unrestricted_guest=0 and 
invalid guest state, but that's it.

Paolo

> For code breakpoint, if the instruction didn't cause vm-exit,
> (for example, the 2nd instruction when kvm emulates instructions
> back to back) emulation.c fails to emulate with DBn.
> 
> For code breakpoint, if the instruction just caused vm-exit.
> It is difficult to analyze this case due to the complex priorities
> between vectored events and fault-like vm-exit.
> Anyway, if it is an instruction that vm-exit has priority over #DB,
> emulation.c fails to emulate with DBn.
> 
> For data breakpoint, a #DB must be delivered to guest or to VMM (when
> guest-debug) after the instruction. But emulation.c doesn't do so.
> 
> And the existence of both of effective DBn (guest debug) and guest DBn
> complicates the problem when we try to emulate them.

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Lai Jiangshan]

On 2021/7/9 17:49, Paolo Bonzini wrote:
> On 09/07/21 05:09, Lai Jiangshan wrote:
>> I just noticed that emulation.c fails to emulate with DBn.
>> Is there any problem around it?
> 
> Just what you said, it's not easy and the needs are limited.  I implemented kvm_vcpu_check_breakpoint because I was 
> interested in using hardware breakpoints from gdb, even with unrestricted_guest=0 and invalid guest state, but that's it.
> 

Hello Paolo

I just remembered I once came across the patch, but I forgot it when
I wrote the mail.

It seems kvm_vcpu_check_breakpoint() handles only for code breakpoint
and doesn't handle for data breakpoints.

And no code handles DR7_GD bit when the emulation is not resulted from
vm-exit. (for example, the non-first instruction when kvm emulates
instructions back to back and the instruction accesses to DBn).

Thanks
Lai


> Paolo
> 
>> For code breakpoint, if the instruction didn't cause vm-exit,
>> (for example, the 2nd instruction when kvm emulates instructions
>> back to back) emulation.c fails to emulate with DBn.
>>
>> For code breakpoint, if the instruction just caused vm-exit.
>> It is difficult to analyze this case due to the complex priorities
>> between vectored events and fault-like vm-exit.
>> Anyway, if it is an instruction that vm-exit has priority over #DB,
>> emulation.c fails to emulate with DBn.
>>
>> For data breakpoint, a #DB must be delivered to guest or to VMM (when
>> guest-debug) after the instruction. But emulation.c doesn't do so.
>>
>> And the existence of both of effective DBn (guest debug) and guest DBn
>> complicates the problem when we try to emulate them.

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Paolo Bonzini]

On 09/07/21 12:05, Lai Jiangshan wrote:
> 
> 
> On 2021/7/9 17:49, Paolo Bonzini wrote:
>> On 09/07/21 05:09, Lai Jiangshan wrote:
>>> I just noticed that emulation.c fails to emulate with DBn.
>>> Is there any problem around it?
>>
>> Just what you said, it's not easy and the needs are limited.  I 
>> implemented kvm_vcpu_check_breakpoint because I was interested in 
>> using hardware breakpoints from gdb, even with unrestricted_guest=0 
>> and invalid guest state, but that's it.
> 
> It seems kvm_vcpu_check_breakpoint() handles only for code breakpoint
> and doesn't handle for data breakpoints.

Correct, there's a comment above the call.  But data breakpoint are much 
harder and relatively less useful.

> And no code handles DR7_GD bit when the emulation is not resulted from
> vm-exit. (for example, the non-first instruction when kvm emulates
> instructions back to back and the instruction accesses to DBn).

Good point, that should be fixed too.

Paolo

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Jim Mattson]

On Fri, Jul 9, 2021 at 8:52 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 09/07/21 12:05, Lai Jiangshan wrote:
> >
> >
> > On 2021/7/9 17:49, Paolo Bonzini wrote:
> >> On 09/07/21 05:09, Lai Jiangshan wrote:
> >>> I just noticed that emulation.c fails to emulate with DBn.
> >>> Is there any problem around it?
> >>
> >> Just what you said, it's not easy and the needs are limited.  I
> >> implemented kvm_vcpu_check_breakpoint because I was interested in
> >> using hardware breakpoints from gdb, even with unrestricted_guest=0
> >> and invalid guest state, but that's it.
> >
> > It seems kvm_vcpu_check_breakpoint() handles only for code breakpoint
> > and doesn't handle for data breakpoints.
>
> Correct, there's a comment above the call.  But data breakpoint are much
> harder and relatively less useful.

Data breakpoints are actually quite useful. I/O breakpoints not so much.

> > And no code handles DR7_GD bit when the emulation is not resulted from
> > vm-exit. (for example, the non-first instruction when kvm emulates
> > instructions back to back and the instruction accesses to DBn).
>
> Good point, that should be fixed too.
>
> Paolo
>

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Paolo Bonzini]

On 09/07/21 18:35, Jim Mattson wrote:
>>>> Just what you said, it's not easy and the needs are limited.  I
>>>> implemented kvm_vcpu_check_breakpoint because I was interested in
>>>> using hardware breakpoints from gdb, even with unrestricted_guest=0
>>>> and invalid guest state, but that's it.
>>> It seems kvm_vcpu_check_breakpoint() handles only for code breakpoint
>>> and doesn't handle for data breakpoints.
>> Correct, there's a comment above the call.  But data breakpoint are much
>> harder and relatively less useful.
> 
> Data breakpoints are actually quite useful. I/O breakpoints not so much.

Normally yes; much less for the specific case of debugging 
invalid-guest-state or other invocations of the emulator.

Paolo

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Jim Mattson]

On Fri, Jul 9, 2021 at 9:59 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 09/07/21 18:35, Jim Mattson wrote:
> >>>> Just what you said, it's not easy and the needs are limited.  I
> >>>> implemented kvm_vcpu_check_breakpoint because I was interested in
> >>>> using hardware breakpoints from gdb, even with unrestricted_guest=0
> >>>> and invalid guest state, but that's it.
> >>> It seems kvm_vcpu_check_breakpoint() handles only for code breakpoint
> >>> and doesn't handle for data breakpoints.
> >> Correct, there's a comment above the call.  But data breakpoint are much
> >> harder and relatively less useful.
> >
> > Data breakpoints are actually quite useful. I/O breakpoints not so much.
>
> Normally yes; much less for the specific case of debugging
> invalid-guest-state or other invocations of the emulator.

Agreed. But if they don't actually work in-guest (because the emulator
ignores them), then their normal usefulness is curtailed.

Re: [PATCH] KVM: X86: Also reload the debug registers before kvm_x86->run() when the host is using them

[Sean Christopherson]

On Thu, Jul 08, 2021, Paolo Bonzini wrote:
> On 28/06/21 19:26, Lai Jiangshan wrote:
> > From: Lai Jiangshan <laijs@linux.alibaba.com>
> > 
> > When the host is using debug registers but the guest is not using them
> > nor is the guest in guest-debug state, the kvm code does not reset
> > the host debug registers before kvm_x86->run().  Rather, it relies on
> > the hardware vmentry instruction to automatically reset the dr7 registers
> > which ensures that the host breakpoints do not affect the guest.
> > 
> > But there are still problems:
> > 	o The addresses of the host breakpoints can leak into the guest
> > 	  and the guest may use these information to attack the host.
> 
> I don't think this is true, because DRn reads would exit (if they don't,
> switch_db_regs would be nonzero).  But otherwise it makes sense to do at
> least the DR7 write, and we might as well do all of them.
> 
> > 	o It violates the non-instrumentable nature around VM entry and
> > 	  exit.  For example, when a host breakpoint is set on
> > 	  vcpu->arch.cr2, #DB will hit aftr kvm_guest_enter_irqoff().
> > 
> > Beside the problems, the logic is not consistent either. When the guest
> > debug registers are active, the host breakpoints are reset before
> > kvm_x86->run(). But when the guest debug registers are inactive, the
> > host breakpoints are delayed to be disabled.  The host tracing tools may
> > see different results depending on there is any guest running or not.
> 
> More precisely, the host tracing tools may see different results depending
> on what the guest is doing.
> 
> Queued (with fixed commit message), thanks!
> 
> Paolo
> 
> > To fix the problems, we also reload the debug registers before
> > kvm_x86->run() when the host is using them whenever the guest is using
> > them or not.
> > 
> > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> > ---
> >   arch/x86/kvm/x86.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b594275d49b5..cce316655d3c 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9320,7 +9320,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >   	if (test_thread_flag(TIF_NEED_FPU_LOAD))
> >   		switch_fpu_return();
> > -	if (unlikely(vcpu->arch.switch_db_regs)) {
> > +	if (unlikely(vcpu->arch.switch_db_regs || hw_breakpoint_active())) {
> >   		set_debugreg(0, 7);

I would prefer zero only dr7, e.g.

	if (unlikely(vcpu->arch.switch_db_regs)) {
		...
	} else if (hw_breakpoint_active()) {
		set_debugreg(0, 7);
	}

Stuffing all DRs isn't a bug because hw_breakpoint_restore() will restore all DRs,
but loading stale state into DRs is weird.

> >   		set_debugreg(vcpu->arch.eff_db[0], 0);
> >   		set_debugreg(vcpu->arch.eff_db[1], 1);
> > 
>