Re: [PATCH v3 08/11] KVM: arm: implement dirty bit mechanism for debug registers
On 2015/7/9 19:50, Christoffer Dall wrote: > On Tue, Jul 07, 2015 at 11:24:06AM +0100, Will Deacon wrote: >> On Tue, Jul 07, 2015 at 11:06:57AM +0100, Zhichao Huang wrote: >>> Chazy and me are talking about how to reduce the saving/restoring >>> overhead for debug registers. >>> We want to add a state in hw_breakpoint.c to indicate whether the host >>> enable any hwbrpts or not (might export a fuction that kvm can call), >>> then we can read this state from memory instead of reading from real >>> hardware registers, and to decide whether we need a world switch or >>> not. >>> Does it acceptable? >> >> Maybe, hard to tell without the code. There are obvious races to deal with >> if you use variables to indicate whether resources are in use -- why not >> just trap debug access from the host as well? Then you could keep track of >> the "owner" in kvm and trap accesses from everybody else. >> > The only information we're looking for here is whether the host has > enabled some break/watch point so that we need to disable them before > running the guest. > > Just to re-iterate, when we are about to run a guest, we have the > following cases: > > 1) Neither the host nor the guest has configured any [WB]points > 2) Only the host has configured any [WB]points > 3) Only the guest has configured any [WB]points > 4) Both the host and the guest have configured any [WB]points > > In case (1), KVM should enable trapping and swtich the register state on > guest accesses. > > In cases (2), (3), and (4) we must switch the register state on each > entry/exit. > > If we are to trap debug register accesses in KVM to set a flag to keep > track of the owner (iow. has the host touched the registers) then don't > we impose an ordering requirement of whether KVM or the breakpoint > functionality gets initialized first, and we need to take special care > when tearing down KVM to disable the traps? It sounds a little complex. > > I've previously suggested to simply look at the B/W control registers to > figure out what to do. Caching the state in memory is an optimization, > do we even have any idea how important such an optimization is? > I have a test for the overhead both in el1 and el2 on D01 board(ARMv7). Each "MRC p14 ..." instruction cost 8 cycles, and Each "MCR p14 ..." cost 5 cycles. A15 has 6 breakpoints and 4 watchpoints, which gives us a total of 20 registers. and the overhead in each world switch is at least (20*8 + 20*5 = 260)cycles. > Thanks, > -Christoffer > -- 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
Re: [PATCH v3 04/11] KVM: arm: common infrastructure for handling AArch32 CP14/CP15
On June 30, 2015 3:43:34 AM GMT+08:00, Christoffer Dall
wrote:
>On Mon, Jun 22, 2015 at 06:41:27PM +0800, Zhichao Huang wrote:
>> As we're about to trap a bunch of CP14 registers, let's rework
>> the CP15 handling so it can be generalized and work with multiple
>> tables.
>>
>> Signed-off-by: Zhichao Huang
>> ---
>> arch/arm/kvm/coproc.c | 176
>++---
>> arch/arm/kvm/interrupts_head.S | 2 +-
>> 2 files changed, 112 insertions(+), 66 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 9d283d9..d23395b 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -375,6 +375,9 @@ static const struct coproc_reg cp15_regs[] = {
>> { CRn(15), CRm( 0), Op1( 4), Op2( 0), is32, access_cbar},
>> };
>>
>> +static const struct coproc_reg cp14_regs[] = {
>> +};
>> +
>> /* Target specific emulation tables */
>> static struct kvm_coproc_target_table
>*target_tables[KVM_ARM_NUM_TARGETS];
>>
>> @@ -424,47 +427,75 @@ static const struct coproc_reg *find_reg(const
>struct coproc_params *params,
>> return NULL;
>> }
>>
>> -static int emulate_cp15(struct kvm_vcpu *vcpu,
>> -const struct coproc_params *params)
>> +/*
>> + * emulate_cp -- tries to match a cp14/cp15 access in a handling
>table,
>> + *and call the corresponding trap handler.
>> + *
>> + * @params: pointer to the descriptor of the access
>> + * @table: array of trap descriptors
>> + * @num: size of the trap descriptor array
>> + *
>> + * Return 0 if the access has been handled, and -1 if not.
>> + */
>> +static int emulate_cp(struct kvm_vcpu *vcpu,
>> +const struct coproc_params *params,
>> +const struct coproc_reg *table,
>> +size_t num)
>> {
>> -size_t num;
>> -const struct coproc_reg *table, *r;
>> -
>> -trace_kvm_emulate_cp15_imp(params->Op1, params->Rt1, params->CRn,
>> - params->CRm, params->Op2, params->is_write);
>> +const struct coproc_reg *r;
>>
>> -table = get_target_table(vcpu->arch.target, &num);
>> +if (!table)
>> +return -1; /* Not handled */
>>
>> -/* Search target-specific then generic table. */
>> r = find_reg(params, table, num);
>> -if (!r)
>> -r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
>>
>> -if (likely(r)) {
>> +if (r) {
>> /* If we don't have an accessor, we should never get here! */
>> BUG_ON(!r->access);
>>
>> if (likely(r->access(vcpu, params, r))) {
>> /* Skip instruction, since it was emulated */
>> kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
>> -return 1;
>> }
>> -/* If access function fails, it should complain. */
>> -} else {
>> -kvm_err("Unsupported guest CP15 access at: %08lx\n",
>> -*vcpu_pc(vcpu));
>> -print_cp_instr(params);
>> +
>> +/* Handled */
>> +return 0;
>> }
>> +
>> +/* Not handled */
>> +return -1;
>> +}
>> +
>> +static void unhandled_cp_access(struct kvm_vcpu *vcpu,
>> +const struct coproc_params *params)
>> +{
>> +u8 hsr_ec = kvm_vcpu_trap_get_class(vcpu);
>> +int cp;
>> +
>> +switch (hsr_ec) {
>> +case HSR_EC_CP15_32:
>> +case HSR_EC_CP15_64:
>> +cp = 15;
>> +break;
>> +case HSR_EC_CP14_MR:
>> +case HSR_EC_CP14_64:
>> +cp = 14;
>> +break;
>> +default:
>> +WARN_ON((cp = -1));
>> +}
>> +
>> +kvm_err("Unsupported guest CP%d access at: %08lx\n",
>> +cp, *vcpu_pc(vcpu));
>> +print_cp_instr(params);
>> kvm_inject_undefined(vcpu);
>> -return 1;
>> }
>>
>> -/**
>> - * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15
>access
>> - * @vcpu: The VCPU pointer
>> - * @run: The kvm_run struct
>> - */
>> -int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>> +const struct coproc_reg *global,
>> +size_t nr_global,
>> +const struct coproc_reg *target_specific,
>> +size_t nr_specific)
>> {
>> struct coproc_params params;
>>
>> @@ -478,7 +509,13 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu,
>struct kvm_run *run)
>> params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
>> params.CRm = 0;
>>
>> -return emulate_cp15(vcpu, ¶ms);
>> +if (!emulate_cp(vcpu, ¶ms, target_specific, nr_specific))
>> +return 1;
>> +if (!emulate_cp(vcpu, ¶ms, global, nr_global))
>> +return 1;
>> +
>> +unhandled_cp_access(vcpu, ¶ms);
>> +return 1;
>> }
>>
>> static void reset_coproc_regs(struct kvm_vcpu *vcpu,
>> @@ -491,12 +528,11 @@
Re: [PATCH v3 06/11] KVM: arm: add trap handlers for 32-bit debug registers
On June 30, 2015 5:16:41 AM GMT+08:00, Christoffer Dall
wrote:
>On Mon, Jun 22, 2015 at 06:41:29PM +0800, Zhichao Huang wrote:
>> Add handlers for all the 32-bit debug registers.
>>
>> Signed-off-by: Zhichao Huang
>> ---
>> arch/arm/include/asm/kvm_asm.h | 12
>> arch/arm/include/asm/kvm_host.h | 3 +
>> arch/arm/kernel/asm-offsets.c | 1 +
>> arch/arm/kvm/coproc.c | 122
>
>> 4 files changed, 138 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/kvm_asm.h
>b/arch/arm/include/asm/kvm_asm.h
>> index 25410b2..ba65e05 100644
>> --- a/arch/arm/include/asm/kvm_asm.h
>> +++ b/arch/arm/include/asm/kvm_asm.h
>> @@ -52,6 +52,18 @@
>> #define c10_AMAIR1 30 /* Auxilary Memory Attribute Indirection Reg1
>*/
>> #define NR_CP15_REGS31 /* Number of regs (incl. invalid) */
>>
>> +/* 0 is reserved as an invalid value. */
>> +#define cp14_DBGBVR01 /* Debug Breakpoint Control Registers
>> (0-15)
>*/
>> +#define cp14_DBGBVR15 16
>> +#define cp14_DBGBCR017 /* Debug Breakpoint Value Registers
>> (0-15)
>*/
>> +#define cp14_DBGBCR15 32
>> +#define cp14_DBGWVR033 /* Debug Watchpoint Control Registers
>> (0-15)
>*/
>> +#define cp14_DBGWVR15 48
>> +#define cp14_DBGWCR049 /* Debug Watchpoint Value Registers
>> (0-15)
>*/
>> +#define cp14_DBGWCR15 64
>> +#define cp14_DBGDSCRext 65 /* Debug Status and Control external */
>> +#define NR_CP14_REGS66 /* Number of regs (incl. invalid) */
>> +
>> #define ARM_EXCEPTION_RESET 0
>> #define ARM_EXCEPTION_UNDEFINED 1
>> #define ARM_EXCEPTION_SOFTWARE2
>> diff --git a/arch/arm/include/asm/kvm_host.h
>b/arch/arm/include/asm/kvm_host.h
>> index d71607c..3d16820 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -124,6 +124,9 @@ struct kvm_vcpu_arch {
>> struct vgic_cpu vgic_cpu;
>> struct arch_timer_cpu timer_cpu;
>>
>> +/* System control coprocessor (cp14) */
>> +u32 cp14[NR_CP14_REGS];
>> +
>> /*
>> * Anything that is not used directly from assembly code goes
>> * here.
>> diff --git a/arch/arm/kernel/asm-offsets.c
>b/arch/arm/kernel/asm-offsets.c
>> index 871b826..9158de0 100644
>> --- a/arch/arm/kernel/asm-offsets.c
>> +++ b/arch/arm/kernel/asm-offsets.c
>> @@ -172,6 +172,7 @@ int main(void)
>> #ifdef CONFIG_KVM_ARM_HOST
>>DEFINE(VCPU_KVM, offsetof(struct kvm_vcpu, kvm));
>>DEFINE(VCPU_MIDR, offsetof(struct kvm_vcpu, arch.midr));
>> + DEFINE(VCPU_CP14, offsetof(struct kvm_vcpu, arch.cp14));
>>DEFINE(VCPU_CP15, offsetof(struct kvm_vcpu, arch.cp15));
>>DEFINE(VCPU_VFP_GUEST,offsetof(struct kvm_vcpu, arch.vfp_guest));
>>DEFINE(VCPU_VFP_HOST, offsetof(struct kvm_vcpu,
>arch.host_cpu_context));
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 16d5f69..59b65b7 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -220,6 +220,47 @@ bool access_vm_reg(struct kvm_vcpu *vcpu,
>> return true;
>> }
>>
>> +static bool trap_debug32(struct kvm_vcpu *vcpu,
>> +const struct coproc_params *p,
>> +const struct coproc_reg *r)
>> +{
>> +if (p->is_write)
>> +vcpu->arch.cp14[r->reg] = *vcpu_reg(vcpu, p->Rt1);
>> +else
>> +*vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp14[r->reg];
>> +
>> +return true;
>> +}
>> +
>> +/* DBGIDR (RO) Debug ID */
>> +static bool trap_dbgidr(struct kvm_vcpu *vcpu,
>> +const struct coproc_params *p,
>> +const struct coproc_reg *r)
>> +{
>> +u32 val;
>> +
>> +if (p->is_write)
>> +return ignore_write(vcpu, p);
>> +
>> +ARM_DBG_READ(c0, c0, 0, val);
>> +*vcpu_reg(vcpu, p->Rt1) = val;
>> +
>> +return true;
>> +}
>> +
>> +/* DBGDSCRint (RO) Debug Status and Control Register */
>> +static bool trap_dbgdscr(struct kvm_vcpu *vcpu,
>> +const struct coproc_params *p,
>> +const struct coproc_reg *r)
>> +{
>> +if (p->is_write)
>> +return ignore_write(vcpu, p);
>> +
>> +*vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp14[r->reg];
>> +
>> +return true;
>> +}
>> +
>> /*
>> * We could trap ID_DFR0 and tell the guest we don't support
>performance
>> * monitoring. Unfortunately the patch to make the kernel check
>ID_DFR0 was
>> @@ -375,7 +416,88 @@ static const struct coproc_reg cp15_regs[] = {
>> { CRn(15), CRm( 0), Op1( 4), Op2( 0), is32, access_cbar},
>> };
>>
>> +#define DBG_BCR_BVR_WCR_WVR(n) \
>> +/* DBGBVRn */ \
>> +{ CRn( 0), CRm((n)), Op1( 0), Op2( 4), is32,\
>> + trap_debug32, reset_val, (cp14_DBGBVR0 + (n)), 0 }, \
>> +/* DBGBCRn */ \
Re: [PATCH v3 01/11] KVM: arm: plug guest debug exploit
On June 29, 2015 11:49:53 PM GMT+08:00, Christoffer Dall
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:
>> Signed-off-by: Zhichao Huang
>> ---
>> 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] ?
>> +
>> +/* 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..357ad1
Re: [PATCH v2 05/11] KVM: arm: check ordering of all system register tables
On 2015/6/10 21:52, Alex Bennée wrote:
>
> Zhichao Huang writes:
>
>> We now have multiple tables for the various system registers
>> we trap. Make sure we check the order of all of them, as it is
>> critical that we get the order right (been there, done that...).
>>
>> Signed-off-by: Zhichao Huang
>> ---
>> arch/arm/kvm/coproc.c | 26 +-
>> 1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index d23395b..16d5f69 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -737,6 +737,9 @@ static struct coproc_reg invariant_cp15[] = {
>> { CRn( 0), CRm( 0), Op1( 0), Op2( 3), is32, NULL, get_TLBTR },
>> { CRn( 0), CRm( 0), Op1( 0), Op2( 6), is32, NULL, get_REVIDR },
>>
>> +{ CRn( 0), CRm( 0), Op1( 1), Op2( 1), is32, NULL, get_CLIDR },
>> +{ CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
>> +
>> { CRn( 0), CRm( 1), Op1( 0), Op2( 0), is32, NULL, get_ID_PFR0 },
>> { CRn( 0), CRm( 1), Op1( 0), Op2( 1), is32, NULL, get_ID_PFR1 },
>> { CRn( 0), CRm( 1), Op1( 0), Op2( 2), is32, NULL, get_ID_DFR0 },
>> @@ -752,9 +755,6 @@ static struct coproc_reg invariant_cp15[] = {
>> { CRn( 0), CRm( 2), Op1( 0), Op2( 3), is32, NULL, get_ID_ISAR3 },
>> { CRn( 0), CRm( 2), Op1( 0), Op2( 4), is32, NULL, get_ID_ISAR4 },
>> { CRn( 0), CRm( 2), Op1( 0), Op2( 5), is32, NULL, get_ID_ISAR5 },
>> -
>> -{ CRn( 0), CRm( 0), Op1( 1), Op2( 1), is32, NULL, get_CLIDR },
>> -{ CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
>> };
>>
>> /*
>> @@ -1297,13 +1297,29 @@ int kvm_arm_copy_coproc_indices(struct kvm_vcpu
>> *vcpu, u64 __user *uindices)
>> return write_demux_regids(uindices);
>> }
>>
>> +static int check_sysreg_table(const struct coproc_reg *table, unsigned int
>> n)
>> +{
>> +unsigned int i;
>> +
>> +for (i = 1; i < n; i++) {
>> +if (cmp_reg(&table[i-1], &table[i]) >= 0) {
>> +kvm_err("sys_reg table %p out of order (%d)\n",
>> +table, i - 1);
>
> Isn't a BUG_ON *and* a kvm_err() overkill?
>
In deed, it would not be able to happened, because all the cp14_regs/cp15_regs
are static codes.
I think the BUG_ON will make the developers to notice whether they get the
order right.
And another reason may be to keep the same way with the ARM64.
>> +return 1;
>> +}
>> +}
>> +
>> +return 0;
>> +}
>> +
>> void kvm_coproc_table_init(void)
>> {
>> unsigned int i;
>>
>> /* Make sure tables are unique and in order. */
>> -for (i = 1; i < ARRAY_SIZE(cp15_regs); i++)
>> -BUG_ON(cmp_reg(&cp15_regs[i-1], &cp15_regs[i]) >= 0);
>> +BUG_ON(check_sysreg_table(cp14_regs, ARRAY_SIZE(cp14_regs)));
>> +BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
>> +BUG_ON(check_sysreg_table(invariant_cp15, ARRAY_SIZE(invariant_cp15)));
>>
>> /* We abuse the reset function to overwrite the table itself. */
>> for (i = 0; i < ARRAY_SIZE(invariant_cp15); i++)
>
--
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
Re: [PATCH v2 04/11] KVM: arm: common infrastructure for handling AArch32 CP14/CP15
On 2015/6/9 18:45, Alex Bennée wrote:
>
> Zhichao Huang writes:
>
>> As we're about to trap a bunch of CP14 registers, let's rework
>> the CP15 handling so it can be generalized and work with multiple
>> tables.
>>
>> Signed-off-by: Zhichao Huang
>> ---
>> arch/arm/kvm/coproc.c | 176
>> ++---
>> arch/arm/kvm/interrupts_head.S | 2 +-
>> 2 files changed, 112 insertions(+), 66 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 9d283d9..d23395b 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -375,6 +375,9 @@ static const struct coproc_reg cp15_regs[] = {
>> { CRn(15), CRm( 0), Op1( 4), Op2( 0), is32, access_cbar},
>> };
>>
>> +static const struct coproc_reg cp14_regs[] = {
>> +};
>> +
>> /* Target specific emulation tables */
>> static struct kvm_coproc_target_table *target_tables[KVM_ARM_NUM_TARGETS];
>>
>> @@ -424,47 +427,75 @@ static const struct coproc_reg *find_reg(const struct
>> coproc_params *params,
>> return NULL;
>> }
>>
>> -static int emulate_cp15(struct kvm_vcpu *vcpu,
>> -const struct coproc_params *params)
>> +/*
>> + * emulate_cp -- tries to match a cp14/cp15 access in a handling table,
>> + *and call the corresponding trap handler.
>> + *
>> + * @params: pointer to the descriptor of the access
>> + * @table: array of trap descriptors
>> + * @num: size of the trap descriptor array
>> + *
>> + * Return 0 if the access has been handled, and -1 if not.
>> + */
>> +static int emulate_cp(struct kvm_vcpu *vcpu,
>> +const struct coproc_params *params,
>> +const struct coproc_reg *table,
>> +size_t num)
>> {
>> -size_t num;
>> -const struct coproc_reg *table, *r;
>> -
>> -trace_kvm_emulate_cp15_imp(params->Op1, params->Rt1, params->CRn,
>> - params->CRm, params->Op2,
>> params->is_write);
>
> Where has this trace gone? We still want to be able to view register
> traps when debugging.
>
OK, I will add it in v3 patches.
>
--
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
Re: [PATCH v2 01/11] KVM: arm: plug guest debug exploit
Hi, Will, I and marc are talking about how to plug the guest debug exploit in an easier way. I remembered that you mentioned disabling monitor mode had proven to be extremely fragile in practice on 32-bit ARM SoCs, what if I save/restore the debug monitor mode on each switch between the guest and the host, would it be acceptable? On 2015/6/15 0:08, zichao wrote: > Hi, Marc, > > On 2015/6/9 18:29, Marc Zyngier wrote: >> On 07/06/15 14:40, zichao wrote: >>> Hi, Marc, >>> >>> On 2015/6/1 18:56, Marc Zyngier wrote: >>>> Hi Zhichao, >>>> >>>> On 31/05/15 05:27, 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 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: >>>>> Signed-off-by: Zhichao Huang >>>>> --- >>>>> 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| 2 +- >>>>> 4 files changed, 59 insertions(+), 10 deletions(-) >>>>> > .. >>>> >>>> There is a small problem here. Imagine the host has programmed a >>>> watchpoint on some VA. We switch to the guest, and then access the same >>>> VA. At that stage, the guest will take the debug exception. That's >>>> really not pretty. >>>> >>> >>> I've thought about it and I think there maybe OK, because when we switch >>> from the >>> host to the guest, the context_switch() in host must be called first, and >>> then >>> the host will switch debug registers, and the guest will not see the >>> watchpoint >>> the host programmed before. >>> >>> Or am I missing some circumstances here? >> >> I don't see anything in this patch that reprograms the debug registers. >> You are simply trapping the guest access to these registers, but >> whatever content the host has put there is still active. >> >> So, assuming that the guest does not touch any debug register (and >> legitimately assumes that they are inactive), a debug exception may fire >> at PL1. >> > > I have had a test on the problem you mentioned. I programmed a watchpoint in > the host, > and then observe the value of debug registers in the guest. > > The result is that in most cases, the guest would not be able to see the > watchpoint because > when we switch from the host to the guest, the process schedule > function(__schedule) would > be called, and it will uninstall debug registers the host just programed. > > __schedule -> __perf_event_task_sched_out -> event_sched_out -> > arch_uninstall_hw_breakpoint > > However, there is one exception, if we programed a watchpoint based on the > Qemu process in > the host, there would be no process schedule between the Qemu process and the > guest, and then, > the problem you mentioned appear, the guest will see the value of debug > registers. > >>>> I think using HDCR_TDE as well should sort it, effectively preventing >>>> the exception from being delivered to the guest, but you will need to >>>> handle this on the HYP side. Performance wise, this is also really >>>> horrible. >>>> >>>> A better way would be to disable the host's BPs/WPs if any is enabled. >> >> I still think you either need to fixup the host's registers if they are >> active when you enter the guest. > > Compared to disable the whole debug feature in the host, I think there may be > a slighter way to > plug the exploit. > > We can only save/restore DBGDSCR on each switch between the guest and the > host. It means that > the debug monitor in guest would be disabled forever(because the guest could > not be able to enable > the debug monitor without the following patches), and then the guest would > not be able to take > any debug exceptions. > > What's your opinion? > >> >> Thanks, >> >> M. >> -- 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
Re: [PATCH v2 01/11] KVM: arm: plug guest debug exploit
Hi, Marc, On 2015/6/9 18:29, Marc Zyngier wrote: > On 07/06/15 14:40, zichao wrote: >> Hi, Marc, >> >> On 2015/6/1 18:56, Marc Zyngier wrote: >>> Hi Zhichao, >>> >>> On 31/05/15 05:27, 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 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: >>>> Signed-off-by: Zhichao Huang >>>> --- >>>> 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| 2 +- >>>> 4 files changed, 59 insertions(+), 10 deletions(-) >>>> .. >>> >>> There is a small problem here. Imagine the host has programmed a >>> watchpoint on some VA. We switch to the guest, and then access the same >>> VA. At that stage, the guest will take the debug exception. That's >>> really not pretty. >>> >> >> I've thought about it and I think there maybe OK, because when we switch >> from the >> host to the guest, the context_switch() in host must be called first, and >> then >> the host will switch debug registers, and the guest will not see the >> watchpoint >> the host programmed before. >> >> Or am I missing some circumstances here? > > I don't see anything in this patch that reprograms the debug registers. > You are simply trapping the guest access to these registers, but > whatever content the host has put there is still active. > > So, assuming that the guest does not touch any debug register (and > legitimately assumes that they are inactive), a debug exception may fire > at PL1. > I have had a test on the problem you mentioned. I programmed a watchpoint in the host, and then observe the value of debug registers in the guest. The result is that in most cases, the guest would not be able to see the watchpoint because when we switch from the host to the guest, the process schedule function(__schedule) would be called, and it will uninstall debug registers the host just programed. __schedule -> __perf_event_task_sched_out -> event_sched_out -> arch_uninstall_hw_breakpoint However, there is one exception, if we programed a watchpoint based on the Qemu process in the host, there would be no process schedule between the Qemu process and the guest, and then, the problem you mentioned appear, the guest will see the value of debug registers. >>> I think using HDCR_TDE as well should sort it, effectively preventing >>> the exception from being delivered to the guest, but you will need to >>> handle this on the HYP side. Performance wise, this is also really horrible. >>> >>> A better way would be to disable the host's BPs/WPs if any is enabled. > > I still think you either need to fixup the host's registers if they are > active when you enter the guest. Compared to disable the whole debug feature in the host, I think there may be a slighter way to plug the exploit. We can only save/restore DBGDSCR on each switch between the guest and the host. It means that the debug monitor in guest would be disabled forever(because the guest could not be able to enable the debug monitor without the following patches), and then the guest would not be able to take any debug exceptions. What's your opinion? > > Thanks, > > M. > -- 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
Re: [PATCH v2 09/11] KVM: arm: disable debug mode if we don't actually need it.
Hi, Will,
On 2015/6/1 18:16, Will Deacon wrote:
> On Sun, May 31, 2015 at 05:27:10AM +0100, Zhichao Huang wrote:
>> Until now we enable debug mode all the time even if we don't
>> actually need it.
>>
>> Inspired by the implementation in arm64, disable debug mode if
>> we don't need it. And then we are able to reduce unnecessary
>> registers saving/restoring when the debug mode is disabled.
>
> I'm terrified about this patch. Enabling monitor mode has proven to be
> *extremely* fragile in practice on 32-bit ARM SoCs, so trying to do this
> morei often makes me very nervous.
>
>> Signed-off-by: Zhichao Huang
>> ---
>> arch/arm/kernel/hw_breakpoint.c | 55
>> ++---
>> 1 file changed, 46 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/kernel/hw_breakpoint.c
>> b/arch/arm/kernel/hw_breakpoint.c
>> index dc7d0a9..1d27563 100644
>> --- a/arch/arm/kernel/hw_breakpoint.c
>> +++ b/arch/arm/kernel/hw_breakpoint.c
>> @@ -266,8 +266,7 @@ static int enable_monitor_mode(void)
>> }
>>
>> /* Check that the write made it through. */
>> -ARM_DBG_READ(c0, c1, 0, dscr);
>> -if (!(dscr & ARM_DSCR_MDBGEN)) {
>> +if (!monitor_mode_enabled()) {
>> pr_warn_once("Failed to enable monitor mode on CPU %d.\n",
>> smp_processor_id());
>> return -EPERM;
>
> Ok, this hunk is harmless :)
>
>> @@ -277,6 +276,43 @@ out:
>> return 0;
>> }
>>
>> +static int disable_monitor_mode(void)
>> +{
>> +u32 dscr;
>> +
>> +ARM_DBG_READ(c0, c1, 0, dscr);
>> +
>> +/* If monitor mode is already disabled, just return. */
>> +if (!(dscr & ARM_DSCR_MDBGEN))
>> +goto out;
>> +
>> +/* Write to the corresponding DSCR. */
>> +switch (get_debug_arch()) {
>> +case ARM_DEBUG_ARCH_V6:
>> +case ARM_DEBUG_ARCH_V6_1:
>> +ARM_DBG_WRITE(c0, c1, 0, (dscr & ~ARM_DSCR_MDBGEN));
>> +break;
>> +case ARM_DEBUG_ARCH_V7_ECP14:
>> +case ARM_DEBUG_ARCH_V7_1:
>> +case ARM_DEBUG_ARCH_V8:
>> +ARM_DBG_WRITE(c0, c2, 2, (dscr & ~ARM_DSCR_MDBGEN));
>> +isb();
>> +break;
>> +default:
>> +return -ENODEV;
>> +}
>> +
>> +/* Check that the write made it through. */
>> +if (monitor_mode_enabled()) {
>> +pr_warn_once("Failed to disable monitor mode on CPU %d.\n",
>> +smp_processor_id());
>> +return -EPERM;
>> +}
>> +
>> +out:
>> +return 0;
>> +}
>
> I'm not comfortable with this. enable_monitor_mode has precisly one caller
> [reset_ctrl_regs] which goes to some lengths to get the system into a
> well-defined state. On top of that, the whole thing is run with an undef
> hook registered because there isn't an architected way to discover whether
> or not DBGSWENABLE is driven low.
>
> Why exactly do you need this? Can you not trap guest debug accesses some
> other way?
OK, I shall look for some other ways to reduce the overhead of world switch.
And another problem might be that, when we start an ARMv7 vm (specify CPU to
be Cortex-A57) on the ARMv8 platform, debug registers will always be
saving/loading
because it is always detected that the debug mode is enabled even though we
didn't acutally use it.
>
>> int hw_breakpoint_slots(int type)
>> {
>> if (!debug_arch_supported())
>> @@ -338,6 +374,8 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>> int i, max_slots, ctrl_base, val_base;
>> u32 addr, ctrl;
>>
>> +enable_monitor_mode();
>> +
>> addr = info->address;
>> ctrl = encode_ctrl_reg(info->ctrl) | 0x1;
>>
>> @@ -430,6 +468,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>>
>> /* Reset the control register. */
>> write_wb_reg(base + i, 0);
>> +
>> +disable_monitor_mode();
>
> My previous concerns notwithstanding, shouldn't this be refcounted?
Maybe shouldn't in my opinion, because we install/uninstall breakpoints only
when
we does context switch, and then we always install/uninstall all the
breakpoints.
>
> Will
>
--
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
Re: [PATCH v2 01/11] KVM: arm: plug guest debug exploit
Hi, Marc,
On 2015/6/1 18:56, Marc Zyngier wrote:
> Hi Zhichao,
>
> On 31/05/15 05:27, 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 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:
>> Signed-off-by: Zhichao Huang
>> ---
>> 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| 2 +-
>> 4 files changed, 59 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;
>> +
>> +/* 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;
>> +
>> +/* 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,
>> +[H
