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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On June 29, 2015 11:49:53 PM GMT+08:00, Christoffer Dall <christoffer.dall@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> Signed-off-by: Zhichao Huang <zhichao.huang@xxxxxxxxxx>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux