On 4 June 2014 17:40, Victor Kamensky <victor.kamensky@xxxxxxxxxx> wrote: > On 4 June 2014 07:53, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote: >> On Thu, May 29, 2014 at 10:33:42PM -0700, Victor Kamensky wrote: >>> Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE >>> image. Before this fix get/set_one_reg functions worked correctly only in >>> LE case - reg_from_user was taking 'void *' kernel address that actually could >>> be target/source memory of either 4 bytes size or 8 bytes size, and code copied >>> from/to user memory that could hold either 4 bytes register, 8 byte register >>> or pair of 4 bytes registers. >>> >>> In order to work in endian agnostic way rule that reg_from_user to reg_to_user >>> should copy register value only to kernel variable with size that matches >>> register size. In few place where size mismatch existed fix issue on macro >>> caller side. >>> >>> Signed-off-by: Victor Kamensky <victor.kamensky@xxxxxxxxxx> >>> --- >>> arch/arm/kvm/coproc.c | 64 +++++++++++++++++++++++++++++++++++++++++++-------- >>> 1 file changed, 55 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c >>> index c58a351..158bcf9 100644 >>> --- a/arch/arm/kvm/coproc.c >>> +++ b/arch/arm/kvm/coproc.c >>> @@ -682,17 +682,23 @@ static struct coproc_reg invariant_cp15[] = { >>> { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR }, >>> }; >>> >>> +/* >>> + * Reads a register value from a userspace address to a kernel >>> + * variable. Make sure that register size matches sizeof(*__val). >>> + */ >>> static int reg_from_user(void *val, const void __user *uaddr, u64 id) >>> { >>> - /* This Just Works because we are little endian. */ >>> if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0) >>> return -EFAULT; >>> return 0; >>> } >>> >>> +/* >>> + * Writes a register value to a userspace address from a kernel variable. >>> + * Make sure that register size matches sizeof(*__val). >>> + */ >>> static int reg_to_user(void __user *uaddr, const void *val, u64 id) >>> { >>> - /* This Just Works because we are little endian. */ >>> if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0) >>> return -EFAULT; >>> return 0; >>> @@ -702,6 +708,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr) >>> { >>> struct coproc_params params; >>> const struct coproc_reg *r; >>> + int ret; >>> >>> if (!index_to_params(id, ¶ms)) >>> return -ENOENT; >>> @@ -710,7 +717,14 @@ static int get_invariant_cp15(u64 id, void __user *uaddr) >>> if (!r) >>> return -ENOENT; >>> >>> - return reg_to_user(uaddr, &r->val, id); >>> + ret = -ENOENT; >>> + if (KVM_REG_SIZE(id) == 4) { >>> + u32 val = r->val; >>> + ret = reg_to_user(uaddr, &val, id); >>> + } else if (KVM_REG_SIZE(id) == 8) { >>> + ret = reg_to_user(uaddr, &r->val, id); >>> + } >>> + return ret; >>> } >>> >>> static int set_invariant_cp15(u64 id, void __user *uaddr) >>> @@ -718,7 +732,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr) >>> struct coproc_params params; >>> const struct coproc_reg *r; >>> int err; >>> - u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */ >>> + u64 val; >>> >>> if (!index_to_params(id, ¶ms)) >>> return -ENOENT; >>> @@ -726,7 +740,15 @@ static int set_invariant_cp15(u64 id, void __user *uaddr) >>> if (!r) >>> return -ENOENT; >>> >>> - err = reg_from_user(&val, uaddr, id); >>> + err = -ENOENT; >>> + if (KVM_REG_SIZE(id) == 4) { >>> + u32 val32; >>> + err = reg_from_user(&val32, uaddr, id); >>> + if (!err) >>> + val = val32; >> >> do you need this conditional? > > With current code, not really, because as next statements go > 'if (err) return err;'. But conditional effectively signify that if user > gave us wrong pointer we would not update val, otherwise if we > do it unconditionally garbage from stack allocated val32 var would > make into val, again currently it is not a problem but if/when > code evolves that may change. Personally I feel it is safer this > way, but I am OK to remove it if you don't like it. Please let > me know. > I don't feel strongly about it, the compiler is probably smart enough to choose an optimal solution anyhow. >>> + } else if (KVM_REG_SIZE(id) == 8) { >>> + err = reg_from_user(&val, uaddr, id); >>> + } >>> if (err) >>> return err; >>> >>> @@ -1004,6 +1026,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >>> { >>> const struct coproc_reg *r; >>> void __user *uaddr = (void __user *)(long)reg->addr; >>> + int ret; >>> >>> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX) >>> return demux_c15_get(reg->id, uaddr); >>> @@ -1015,14 +1038,25 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >>> if (!r) >>> return get_invariant_cp15(reg->id, uaddr); >>> >>> - /* Note: copies two regs if size is 64 bit. */ >>> - return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id); >>> + ret = -ENOENT; >>> + if (KVM_REG_SIZE(reg->id) == 8) { >>> + u64 val; >>> + /* Note: copies two u32 regs */ >>> + val = (vcpu->arch.cp15[r->reg + 1] << 32) | >>> + vcpu->arch.cp15[r->reg]; >>> + ret = reg_to_user(uaddr, &val, reg->id); >>> + } else if (KVM_REG_SIZE(reg->id) == 4) { >>> + ret = reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id); >>> + } >>> + >>> + return ret; >>> } >>> >>> int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >>> { >>> const struct coproc_reg *r; >>> void __user *uaddr = (void __user *)(long)reg->addr; >>> + int ret; >>> >>> if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX) >>> return demux_c15_set(reg->id, uaddr); >>> @@ -1034,8 +1068,20 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >>> if (!r) >>> return set_invariant_cp15(reg->id, uaddr); >>> >>> - /* Note: copies two regs if size is 64 bit */ >>> - return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id); >>> + ret = -ENOENT; >>> + if (KVM_REG_SIZE(reg->id) == 8) { >>> + u64 val; >>> + /* Note: copies two u32 regs */ >>> + ret = reg_from_user(&val, uaddr, reg->id); >>> + if (!ret) { >>> + vcpu->arch.cp15[r->reg] = val & 0xffffffff; >>> + vcpu->arch.cp15[r->reg + 1] = val >> 32; >>> + } >>> + } else if (KVM_REG_SIZE(reg->id) == 4) { >>> + ret = reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id); >>> + } >>> + >>> + return ret; >>> } >>> >>> static unsigned int num_demux_regs(void) >>> -- >>> 1.8.1.4 >>> >> >> Yep, much cleaner than before. > > Thanks. > > I'll try to wrap up pieces like > >>> + vcpu->arch.cp15[r->reg] = val & 0xffffffff; >>> + vcpu->arch.cp15[r->reg + 1] = val >> 32; > > into inline function as you suggested in other email to see whether > it will look better. yeah, I'm not 100% convinced that it will be cleaner with an inline function, this does look somewhat clean, so depends on how it looks like. > > And after that, I'll run a lot of testing for my current series and report > the whole series again. > Awesome, thanks. -Christoffer _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm