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.d...@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: <sta...@vger.kernel.org>
> >> Signed-off-by: Zhichao Huang <zhichao.hu...@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, ¶ms);
> >> }
> >>
> >> +/**
> >> + * 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, ¶ms, 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, ¶ms, 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
