[PATCH v3 01/11] KVM: arm: plug guest debug exploit

[PATCH v3 01/11] KVM: arm: plug guest debug exploit

[Zhichao Huang]

Hardware debugging in guests is not intercepted currently, it means
that a malicious guest can bring down the entire machine by writing
to the debug registers.

This patch enable trapping of all debug registers, preventing the guests
to access the debug registers.

This patch also disable the debug mode(DBGDSCR) in the guest world all
the time, preventing the guests to mess with the host state.

However, it is a precursor for later patches which will need to do
more to world switch debug states while necessary.

Cc: <stable@vger.kernel.org>
Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/include/asm/kvm_coproc.h |  3 +-
 arch/arm/kvm/coproc.c             | 60 +++++++++++++++++++++++++++++++++++----
 arch/arm/kvm/handle_exit.c        |  4 +--
 arch/arm/kvm/interrupts_head.S    | 13 ++++++++-
 4 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/arch/arm/include/asm/kvm_coproc.h b/arch/arm/include/asm/kvm_coproc.h
index 4917c2f..e74ab0f 100644
--- a/arch/arm/include/asm/kvm_coproc.h
+++ b/arch/arm/include/asm/kvm_coproc.h
@@ -31,7 +31,8 @@ void kvm_register_target_coproc_table(struct kvm_coproc_target_table *table);
 int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp_0_13_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run);
-int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
+int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
+int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
 
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index f3d88dc..2e12760 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -91,12 +91,6 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
-int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run)
-{
-	kvm_inject_undefined(vcpu);
-	return 1;
-}
-
 static void reset_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *r)
 {
 	/*
@@ -519,6 +513,60 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return emulate_cp15(vcpu, &params);
 }
 
+/**
+ * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	struct coproc_params params;
+
+	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
+	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
+	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
+	params.is_64bit = true;
+
+	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 16) & 0xf;
+	params.Op2 = 0;
+	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
+	params.CRm = 0;
+
+	/* raz_wi */
+	(void)pm_fake(vcpu, &params, NULL);
+
+	/* handled */
+	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+	return 1;
+}
+
+/**
+ * kvm_handle_cp14_32 -- handles a mrc/mcr trap on a guest CP14 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	struct coproc_params params;
+
+	params.CRm = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
+	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
+	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
+	params.is_64bit = false;
+
+	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
+	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 14) & 0x7;
+	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
+	params.Rt2 = 0;
+
+	/* raz_wi */
+	(void)pm_fake(vcpu, &params, NULL);
+
+	/* handled */
+	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+	return 1;
+}
+
 /******************************************************************************
  * Userspace API
  *****************************************************************************/
diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
index 95f12b2..357ad1b 100644
--- a/arch/arm/kvm/handle_exit.c
+++ b/arch/arm/kvm/handle_exit.c
@@ -104,9 +104,9 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[HSR_EC_WFI]		= kvm_handle_wfx,
 	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
 	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
-	[HSR_EC_CP14_MR]	= kvm_handle_cp14_access,
+	[HSR_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[HSR_EC_CP14_LS]	= kvm_handle_cp14_load_store,
-	[HSR_EC_CP14_64]	= kvm_handle_cp14_access,
+	[HSR_EC_CP14_64]	= kvm_handle_cp14_64,
 	[HSR_EC_CP_0_13]	= kvm_handle_cp_0_13_access,
 	[HSR_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[HSR_EC_SVC_HYP]	= handle_svc_hyp,
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 35e4a3a..f85c447 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -97,6 +97,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	mrs	r8, LR_fiq
 	mrs	r9, SPSR_fiq
 	push	{r2-r9}
+
+	/* DBGDSCR reg */
+	mrc	p14, 0, r2, c0, c1, 0
+	push	{r2}
 .endm
 
 .macro pop_host_regs_mode mode
@@ -111,6 +115,9 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * Clobbers all registers, in all modes, except r0 and r1.
  */
 .macro restore_host_regs
+	pop	{r2}
+	mcr	p14, 0, r2, c0, c2, 2
+
 	pop	{r2-r9}
 	msr	r8_fiq, r2
 	msr	r9_fiq, r3
@@ -159,6 +166,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * Clobbers *all* registers.
  */
 .macro restore_guest_regs
+	/* reset DBGDSCR to disable debug mode */
+	mov	r2, #0
+	mcr	p14, 0, r2, c0, c2, 2
+
 	restore_guest_regs_mode svc, #VCPU_SVC_REGS
 	restore_guest_regs_mode abt, #VCPU_ABT_REGS
 	restore_guest_regs_mode und, #VCPU_UND_REGS
@@ -607,7 +618,7 @@ ARM_BE8(rev	r6, r6  )
  * (hardware reset value is 0) */
 .macro set_hdcr operation
 	mrc	p15, 4, r2, c1, c1, 1
-	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
+	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
 	.if \operation == vmentry
 	orr	r2, r2, r3		@ Trap some perfmon accesses
 	.else
-- 
1.7.12.4

--
To unsubscribe from this list: send the line "unsubscribe stable" in

Re: [PATCH v3 01/11] KVM: arm: plug guest debug exploit

[Christoffer Dall]

On Mon, Jun 22, 2015 at 06:41:24PM +0800, Zhichao Huang wrote:
> Hardware debugging in guests is not intercepted currently, it means
> that a malicious guest can bring down the entire machine by writing
> to the debug registers.
> 
> This patch enable trapping of all debug registers, preventing the guests
> to access the debug registers.
> 
> This patch also disable the debug mode(DBGDSCR) in the guest world all
> the time, preventing the guests to mess with the host state.
> 
> However, it is a precursor for later patches which will need to do
> more to world switch debug states while necessary.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/include/asm/kvm_coproc.h |  3 +-
>  arch/arm/kvm/coproc.c             | 60 +++++++++++++++++++++++++++++++++++----
>  arch/arm/kvm/handle_exit.c        |  4 +--
>  arch/arm/kvm/interrupts_head.S    | 13 ++++++++-
>  4 files changed, 70 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_coproc.h b/arch/arm/include/asm/kvm_coproc.h
> index 4917c2f..e74ab0f 100644
> --- a/arch/arm/include/asm/kvm_coproc.h
> +++ b/arch/arm/include/asm/kvm_coproc.h
> @@ -31,7 +31,8 @@ void kvm_register_target_coproc_table(struct kvm_coproc_target_table *table);
>  int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp_0_13_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run);
> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index f3d88dc..2e12760 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -91,12 +91,6 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run)
> -{
> -	kvm_inject_undefined(vcpu);
> -	return 1;
> -}
> -
>  static void reset_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *r)
>  {
>  	/*
> @@ -519,6 +513,60 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return emulate_cp15(vcpu, &params);
>  }
>  
> +/**
> + * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	struct coproc_params params;
> +
> +	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
> +	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
> +	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
> +	params.is_64bit = true;
> +
> +	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 16) & 0xf;
> +	params.Op2 = 0;
> +	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
> +	params.CRm = 0;

this is a complete duplicate of kvm_handle_cp15_64, can you share this
code somehow?

> +
> +	/* raz_wi */
> +	(void)pm_fake(vcpu, &params, NULL);
> +
> +	/* handled */
> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +	return 1;
> +}
> +
> +/**
> + * kvm_handle_cp14_32 -- handles a mrc/mcr trap on a guest CP14 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	struct coproc_params params;
> +
> +	params.CRm = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
> +	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
> +	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
> +	params.is_64bit = false;
> +
> +	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
> +	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 14) & 0x7;
> +	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
> +	params.Rt2 = 0;

this is a complete duplicate of kvm_handle_cp15_32, can you share this
code somehow?

> +
> +	/* raz_wi */
> +	(void)pm_fake(vcpu, &params, NULL);
> +
> +	/* handled */
> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +	return 1;
> +}
> +
>  /******************************************************************************
>   * Userspace API
>   *****************************************************************************/
> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
> index 95f12b2..357ad1b 100644
> --- a/arch/arm/kvm/handle_exit.c
> +++ b/arch/arm/kvm/handle_exit.c
> @@ -104,9 +104,9 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[HSR_EC_WFI]		= kvm_handle_wfx,
>  	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
>  	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
> -	[HSR_EC_CP14_MR]	= kvm_handle_cp14_access,
> +	[HSR_EC_CP14_MR]	= kvm_handle_cp14_32,
>  	[HSR_EC_CP14_LS]	= kvm_handle_cp14_load_store,
> -	[HSR_EC_CP14_64]	= kvm_handle_cp14_access,
> +	[HSR_EC_CP14_64]	= kvm_handle_cp14_64,
>  	[HSR_EC_CP_0_13]	= kvm_handle_cp_0_13_access,
>  	[HSR_EC_CP10_ID]	= kvm_handle_cp10_id,
>  	[HSR_EC_SVC_HYP]	= handle_svc_hyp,
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 35e4a3a..f85c447 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -97,6 +97,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	mrs	r8, LR_fiq
>  	mrs	r9, SPSR_fiq
>  	push	{r2-r9}
> +
> +	/* DBGDSCR reg */
> +	mrc	p14, 0, r2, c0, c1, 0
> +	push	{r2}

this feels like it should belong in read_cp15_state and not the gp regs
portion ?


>  .endm
>  
>  .macro pop_host_regs_mode mode
> @@ -111,6 +115,9 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   * Clobbers all registers, in all modes, except r0 and r1.
>   */
>  .macro restore_host_regs
> +	pop	{r2}
> +	mcr	p14, 0, r2, c0, c2, 2
> +

Why are we reading the DBGDSCRint and writing the DBGDSCRext ?

>  	pop	{r2-r9}
>  	msr	r8_fiq, r2
>  	msr	r9_fiq, r3
> @@ -159,6 +166,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   * Clobbers *all* registers.
>   */
>  .macro restore_guest_regs
> +	/* reset DBGDSCR to disable debug mode */
> +	mov	r2, #0
> +	mcr	p14, 0, r2, c0, c2, 2

Is it valid to write 0 in all all fields of this register?

I thought Will expressed concern about accessing this register?  Why is
it safe in this context and not before?  It seems from the spec that
this can still raise an undefined exception if an external debugger
lowers the software debug enable signal.

> +
>  	restore_guest_regs_mode svc, #VCPU_SVC_REGS
>  	restore_guest_regs_mode abt, #VCPU_ABT_REGS
>  	restore_guest_regs_mode und, #VCPU_UND_REGS
> @@ -607,7 +618,7 @@ ARM_BE8(rev	r6, r6  )
>   * (hardware reset value is 0) */
>  .macro set_hdcr operation
>  	mrc	p15, 4, r2, c1, c1, 1
> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
>  	.if \operation == vmentry
>  	orr	r2, r2, r3		@ Trap some perfmon accesses
>  	.else
> -- 
> 1.7.12.4
> 

Thanks,
-Christoffer

Re: [PATCH v3 01/11] KVM: arm: plug guest debug exploit

[zichao]

On June 29, 2015 11:49:53 PM GMT+08:00, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>On Mon, Jun 22, 2015 at 06:41:24PM +0800, Zhichao Huang wrote:
>> Hardware debugging in guests is not intercepted currently, it means
>> that a malicious guest can bring down the entire machine by writing
>> to the debug registers.
>> 
>> This patch enable trapping of all debug registers, preventing the
>guests
>> to access the debug registers.
>> 
>> This patch also disable the debug mode(DBGDSCR) in the guest world
>all
>> the time, preventing the guests to mess with the host state.
>> 
>> However, it is a precursor for later patches which will need to do
>> more to world switch debug states while necessary.
>> 
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
>> ---
>>  arch/arm/include/asm/kvm_coproc.h |  3 +-
>>  arch/arm/kvm/coproc.c             | 60
>+++++++++++++++++++++++++++++++++++----
>>  arch/arm/kvm/handle_exit.c        |  4 +--
>>  arch/arm/kvm/interrupts_head.S    | 13 ++++++++-
>>  4 files changed, 70 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/arm/include/asm/kvm_coproc.h
>b/arch/arm/include/asm/kvm_coproc.h
>> index 4917c2f..e74ab0f 100644
>> --- a/arch/arm/include/asm/kvm_coproc.h
>> +++ b/arch/arm/include/asm/kvm_coproc.h
>> @@ -31,7 +31,8 @@ void kvm_register_target_coproc_table(struct
>kvm_coproc_target_table *table);
>>  int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>  int kvm_handle_cp_0_13_access(struct kvm_vcpu *vcpu, struct kvm_run
>*run);
>>  int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run
>*run);
>> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run
>*run);
>> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>  
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index f3d88dc..2e12760 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -91,12 +91,6 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu
>*vcpu, struct kvm_run *run)
>>  	return 1;
>>  }
>>  
>> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run
>*run)
>> -{
>> -	kvm_inject_undefined(vcpu);
>> -	return 1;
>> -}
>> -
>>  static void reset_mpidr(struct kvm_vcpu *vcpu, const struct
>coproc_reg *r)
>>  {
>>  	/*
>> @@ -519,6 +513,60 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu,
>struct kvm_run *run)
>>  	return emulate_cp15(vcpu, &params);
>>  }
>>  
>> +/**
>> + * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14
>access
>> + * @vcpu: The VCPU pointer
>> + * @run:  The kvm_run struct
>> + */
>> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> +	struct coproc_params params;
>> +
>> +	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
>> +	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
>> +	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
>> +	params.is_64bit = true;
>> +
>> +	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 16) & 0xf;
>> +	params.Op2 = 0;
>> +	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
>> +	params.CRm = 0;
>
>this is a complete duplicate of kvm_handle_cp15_64, can you share this
>code somehow?
>

This patch just want to plug the exploit in the simplest way, and I shared the cp14/cp15 handlers in later patches [PATCH v3 04/11].

Should I take the patch [04/11] ahead of current patch [01/11] ?

>> +
>> +	/* raz_wi */
>> +	(void)pm_fake(vcpu, &params, NULL);
>> +
>> +	/* handled */
>> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
>> +	return 1;
>> +}
>> +
>> +/**
>> + * kvm_handle_cp14_32 -- handles a mrc/mcr trap on a guest CP14
>access
>> + * @vcpu: The VCPU pointer
>> + * @run:  The kvm_run struct
>> + */
>> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> +	struct coproc_params params;
>> +
>> +	params.CRm = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
>> +	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
>> +	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
>> +	params.is_64bit = false;
>> +
>> +	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
>> +	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 14) & 0x7;
>> +	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
>> +	params.Rt2 = 0;
>
>this is a complete duplicate of kvm_handle_cp15_32, can you share this
>code somehow?
>
>> +
>> +	/* raz_wi */
>> +	(void)pm_fake(vcpu, &params, NULL);
>> +
>> +	/* handled */
>> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
>> +	return 1;
>> +}
>> +
>> 
>/******************************************************************************
>>   * Userspace API
>>  
>*****************************************************************************/
>> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
>> index 95f12b2..357ad1b 100644
>> --- a/arch/arm/kvm/handle_exit.c
>> +++ b/arch/arm/kvm/handle_exit.c
>> @@ -104,9 +104,9 @@ static exit_handle_fn arm_exit_handlers[] = {
>>  	[HSR_EC_WFI]		= kvm_handle_wfx,
>>  	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
>>  	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
>> -	[HSR_EC_CP14_MR]	= kvm_handle_cp14_access,
>> +	[HSR_EC_CP14_MR]	= kvm_handle_cp14_32,
>>  	[HSR_EC_CP14_LS]	= kvm_handle_cp14_load_store,
>> -	[HSR_EC_CP14_64]	= kvm_handle_cp14_access,
>> +	[HSR_EC_CP14_64]	= kvm_handle_cp14_64,
>>  	[HSR_EC_CP_0_13]	= kvm_handle_cp_0_13_access,
>>  	[HSR_EC_CP10_ID]	= kvm_handle_cp10_id,
>>  	[HSR_EC_SVC_HYP]	= handle_svc_hyp,
>> diff --git a/arch/arm/kvm/interrupts_head.S
>b/arch/arm/kvm/interrupts_head.S
>> index 35e4a3a..f85c447 100644
>> --- a/arch/arm/kvm/interrupts_head.S
>> +++ b/arch/arm/kvm/interrupts_head.S
>> @@ -97,6 +97,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>>  	mrs	r8, LR_fiq
>>  	mrs	r9, SPSR_fiq
>>  	push	{r2-r9}
>> +
>> +	/* DBGDSCR reg */
>> +	mrc	p14, 0, r2, c0, c1, 0
>> +	push	{r2}
>
>this feels like it should belong in read_cp15_state and not the gp regs
>portion ?
>

Happy to move it. But moving the cp14 regs to read/write_cp15_state still seems no very appropriate. Should I move it to __kvm_vcpu_return and __kvm_vcpu_run?

Another reason might be that, I want to disable debug mode (DBGDSCR) as early as possible.

>
>>  .endm
>>  
>>  .macro pop_host_regs_mode mode
>> @@ -111,6 +115,9 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>>   * Clobbers all registers, in all modes, except r0 and r1.
>>   */
>>  .macro restore_host_regs
>> +	pop	{r2}
>> +	mcr	p14, 0, r2, c0, c2, 2
>> +
>
>Why are we reading the DBGDSCRint and writing the DBGDSCRext ?

Because the DBGDSCRint is read-only, and I borrowed the operation from kernel.

arch/arm/kernel/hw_breakpoint.c:
ARM_DBG_READ(c0, c1, 0, dscr)
ARM_DBG_WRITE(c0, c2, 2, dscr)
>
>>  	pop	{r2-r9}
>>  	msr	r8_fiq, r2
>>  	msr	r9_fiq, r3
>> @@ -159,6 +166,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>>   * Clobbers *all* registers.
>>   */
>>  .macro restore_guest_regs
>> +	/* reset DBGDSCR to disable debug mode */
>> +	mov	r2, #0
>> +	mcr	p14, 0, r2, c0, c2, 2
>
>Is it valid to write 0 in all all fields of this register?

I'm afraid of it too, although it tests ok. Does Will have any suggestions?
>
>I thought Will expressed concern about accessing this register?  Why is
>it safe in this context and not before?  It seems from the spec that
>this can still raise an undefined exception if an external debugger
>lowers the software debug enable signal.
>
>> +
>>  	restore_guest_regs_mode svc, #VCPU_SVC_REGS
>>  	restore_guest_regs_mode abt, #VCPU_ABT_REGS
>>  	restore_guest_regs_mode und, #VCPU_UND_REGS
>> @@ -607,7 +618,7 @@ ARM_BE8(rev	r6, r6  )
>>   * (hardware reset value is 0) */
>>  .macro set_hdcr operation
>>  	mrc	p15, 4, r2, c1, c1, 1
>> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
>> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
>>  	.if \operation == vmentry
>>  	orr	r2, r2, r3		@ Trap some perfmon accesses
>>  	.else
>> -- 
>> 1.7.12.4
>> 
>
>Thanks,
>-Christoffer

-- 
zhichao.huang

Re: [PATCH v3 01/11] KVM: arm: plug guest debug exploit

[Christoffer Dall]

On Wed, Jul 01, 2015 at 03:04:00PM +0800, zichao wrote:
> 
> 
> On June 29, 2015 11:49:53 PM GMT+08:00, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> >On Mon, Jun 22, 2015 at 06:41:24PM +0800, Zhichao Huang wrote:
> >> Hardware debugging in guests is not intercepted currently, it means
> >> that a malicious guest can bring down the entire machine by writing
> >> to the debug registers.
> >> 
> >> This patch enable trapping of all debug registers, preventing the
> >guests
> >> to access the debug registers.
> >> 
> >> This patch also disable the debug mode(DBGDSCR) in the guest world
> >all
> >> the time, preventing the guests to mess with the host state.
> >> 
> >> However, it is a precursor for later patches which will need to do
> >> more to world switch debug states while necessary.
> >> 
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> >> ---
> >>  arch/arm/include/asm/kvm_coproc.h |  3 +-
> >>  arch/arm/kvm/coproc.c             | 60
> >+++++++++++++++++++++++++++++++++++----
> >>  arch/arm/kvm/handle_exit.c        |  4 +--
> >>  arch/arm/kvm/interrupts_head.S    | 13 ++++++++-
> >>  4 files changed, 70 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/arch/arm/include/asm/kvm_coproc.h
> >b/arch/arm/include/asm/kvm_coproc.h
> >> index 4917c2f..e74ab0f 100644
> >> --- a/arch/arm/include/asm/kvm_coproc.h
> >> +++ b/arch/arm/include/asm/kvm_coproc.h
> >> @@ -31,7 +31,8 @@ void kvm_register_target_coproc_table(struct
> >kvm_coproc_target_table *table);
> >>  int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >>  int kvm_handle_cp_0_13_access(struct kvm_vcpu *vcpu, struct kvm_run
> >*run);
> >>  int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run
> >*run);
> >> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run
> >*run);
> >> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >>  
> >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> >> index f3d88dc..2e12760 100644
> >> --- a/arch/arm/kvm/coproc.c
> >> +++ b/arch/arm/kvm/coproc.c
> >> @@ -91,12 +91,6 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu
> >*vcpu, struct kvm_run *run)
> >>  	return 1;
> >>  }
> >>  
> >> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run
> >*run)
> >> -{
> >> -	kvm_inject_undefined(vcpu);
> >> -	return 1;
> >> -}
> >> -
> >>  static void reset_mpidr(struct kvm_vcpu *vcpu, const struct
> >coproc_reg *r)
> >>  {
> >>  	/*
> >> @@ -519,6 +513,60 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu,
> >struct kvm_run *run)
> >>  	return emulate_cp15(vcpu, &params);
> >>  }
> >>  
> >> +/**
> >> + * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14
> >access
> >> + * @vcpu: The VCPU pointer
> >> + * @run:  The kvm_run struct
> >> + */
> >> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >> +{
> >> +	struct coproc_params params;
> >> +
> >> +	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
> >> +	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
> >> +	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
> >> +	params.is_64bit = true;
> >> +
> >> +	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 16) & 0xf;
> >> +	params.Op2 = 0;
> >> +	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
> >> +	params.CRm = 0;
> >
> >this is a complete duplicate of kvm_handle_cp15_64, can you share this
> >code somehow?
> >
> 
> This patch just want to plug the exploit in the simplest way, and I shared the cp14/cp15 handlers in later patches [PATCH v3 04/11].
> 
> Should I take the patch [04/11] ahead of current patch [01/11] ?
> 

It would be good if the patch that we can cc stable and which fixes the
issue is self-contained.  If it's impossible to do that while sharing
the handlers (I don't see why, but I didn't write the code) then ok, but
otherwise just add that bit of code into this patch I would say.

> >> +
> >> +	/* raz_wi */
> >> +	(void)pm_fake(vcpu, &params, NULL);
> >> +
> >> +	/* handled */
> >> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> >> +	return 1;
> >> +}
> >> +
> >> +/**
> >> + * kvm_handle_cp14_32 -- handles a mrc/mcr trap on a guest CP14
> >access
> >> + * @vcpu: The VCPU pointer
> >> + * @run:  The kvm_run struct
> >> + */
> >> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >> +{
> >> +	struct coproc_params params;
> >> +
> >> +	params.CRm = (kvm_vcpu_get_hsr(vcpu) >> 1) & 0xf;
> >> +	params.Rt1 = (kvm_vcpu_get_hsr(vcpu) >> 5) & 0xf;
> >> +	params.is_write = ((kvm_vcpu_get_hsr(vcpu) & 1) == 0);
> >> +	params.is_64bit = false;
> >> +
> >> +	params.CRn = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
> >> +	params.Op1 = (kvm_vcpu_get_hsr(vcpu) >> 14) & 0x7;
> >> +	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
> >> +	params.Rt2 = 0;
> >
> >this is a complete duplicate of kvm_handle_cp15_32, can you share this
> >code somehow?
> >
> >> +
> >> +	/* raz_wi */
> >> +	(void)pm_fake(vcpu, &params, NULL);
> >> +
> >> +	/* handled */
> >> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> >> +	return 1;
> >> +}
> >> +
> >> 
> >/******************************************************************************
> >>   * Userspace API
> >>  
> >*****************************************************************************/
> >> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
> >> index 95f12b2..357ad1b 100644
> >> --- a/arch/arm/kvm/handle_exit.c
> >> +++ b/arch/arm/kvm/handle_exit.c
> >> @@ -104,9 +104,9 @@ static exit_handle_fn arm_exit_handlers[] = {
> >>  	[HSR_EC_WFI]		= kvm_handle_wfx,
> >>  	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
> >>  	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
> >> -	[HSR_EC_CP14_MR]	= kvm_handle_cp14_access,
> >> +	[HSR_EC_CP14_MR]	= kvm_handle_cp14_32,
> >>  	[HSR_EC_CP14_LS]	= kvm_handle_cp14_load_store,
> >> -	[HSR_EC_CP14_64]	= kvm_handle_cp14_access,
> >> +	[HSR_EC_CP14_64]	= kvm_handle_cp14_64,
> >>  	[HSR_EC_CP_0_13]	= kvm_handle_cp_0_13_access,
> >>  	[HSR_EC_CP10_ID]	= kvm_handle_cp10_id,
> >>  	[HSR_EC_SVC_HYP]	= handle_svc_hyp,
> >> diff --git a/arch/arm/kvm/interrupts_head.S
> >b/arch/arm/kvm/interrupts_head.S
> >> index 35e4a3a..f85c447 100644
> >> --- a/arch/arm/kvm/interrupts_head.S
> >> +++ b/arch/arm/kvm/interrupts_head.S
> >> @@ -97,6 +97,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
> >>  	mrs	r8, LR_fiq
> >>  	mrs	r9, SPSR_fiq
> >>  	push	{r2-r9}
> >> +
> >> +	/* DBGDSCR reg */
> >> +	mrc	p14, 0, r2, c0, c1, 0
> >> +	push	{r2}
> >
> >this feels like it should belong in read_cp15_state and not the gp regs
> >portion ?
> >
> 
> Happy to move it. But moving the cp14 regs to read/write_cp15_state still seems no very appropriate. Should I move it to __kvm_vcpu_return and __kvm_vcpu_run?

you should probably rename read_cp15_state to read_coproc_state then.

> 
> Another reason might be that, I want to disable debug mode (DBGDSCR) as early as possible.
> 

Why?  The world-switch code is atomic in that sense is it not?

> >
> >>  .endm
> >>  
> >>  .macro pop_host_regs_mode mode
> >> @@ -111,6 +115,9 @@ vcpu	.req	r0		@ vcpu pointer always in r0
> >>   * Clobbers all registers, in all modes, except r0 and r1.
> >>   */
> >>  .macro restore_host_regs
> >> +	pop	{r2}
> >> +	mcr	p14, 0, r2, c0, c2, 2
> >> +
> >
> >Why are we reading the DBGDSCRint and writing the DBGDSCRext ?
> 
> Because the DBGDSCRint is read-only, and I borrowed the operation from kernel.
> 
> arch/arm/kernel/hw_breakpoint.c:
> ARM_DBG_READ(c0, c1, 0, dscr)
> ARM_DBG_WRITE(c0, c2, 2, dscr)
> >
> >>  	pop	{r2-r9}
> >>  	msr	r8_fiq, r2
> >>  	msr	r9_fiq, r3
> >> @@ -159,6 +166,10 @@ vcpu	.req	r0		@ vcpu pointer always in r0
> >>   * Clobbers *all* registers.
> >>   */
> >>  .macro restore_guest_regs
> >> +	/* reset DBGDSCR to disable debug mode */
> >> +	mov	r2, #0
> >> +	mcr	p14, 0, r2, c0, c2, 2
> >
> >Is it valid to write 0 in all all fields of this register?
> 
> I'm afraid of it too, although it tests ok. Does Will have any suggestions?
> >
> >I thought Will expressed concern about accessing this register?  Why is
> >it safe in this context and not before?  It seems from the spec that
> >this can still raise an undefined exception if an external debugger
> >lowers the software debug enable signal.
> >
> >> +
> >>  	restore_guest_regs_mode svc, #VCPU_SVC_REGS
> >>  	restore_guest_regs_mode abt, #VCPU_ABT_REGS
> >>  	restore_guest_regs_mode und, #VCPU_UND_REGS
> >> @@ -607,7 +618,7 @@ ARM_BE8(rev	r6, r6  )
> >>   * (hardware reset value is 0) */
> >>  .macro set_hdcr operation
> >>  	mrc	p15, 4, r2, c1, c1, 1
> >> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
> >> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
> >>  	.if \operation == vmentry
> >>  	orr	r2, r2, r3		@ Trap some perfmon accesses
> >>  	.else
> >> -- 
> >> 1.7.12.4
> >> 
> >
> >Thanks,
> >-Christoffer
> 
> -- 
> zhichao.huang