On Tue, May 27, 2014 at 11:08:11PM -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.
>
> For example note that there was a case when 4 bytes register was read from
> userspace to kernel target address of 8 bytes value. Because it was working
> in LE, least significant word was memcpy(ied) and it just worked. In BE code
> with 'void *' as target/source 'val' type it is impossible to tell whether
> 4 bytes register from userspace should be copied to 'val' address itself
> (4 bytes target) or it should be copied to 'val' + 4 (least significant word
> of 8 bytes value). So first change was to introduce strongly typed
> functions, where type of target/source 'val' is strongly defined:
>
> reg_from_user_to_u64 - reads register from userspace to kernel 'u64 *val'
> address; register size must be 8 bytes
> reg_from_user_to_u32 - reads register(s) from userspace to kernel 'u32 *val'
> address; note it could be one or two 4 bytes registers
> reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to
> userspace register memory; register size must be 8 bytes
> ret_to_user_from_u32 - writes register(s) from kernel 'u32 *val' address to
> userspace register(s) memory; note it could be
> one or two 4 bytes registers
>
> All places where reg_from_user, reg_to_user functions were used, were changed
> to use either corresponding u64 or u32 variants of functions depending on
> type of source/target kernel memory variable.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@xxxxxxxxxx>
> ---
> arch/arm/kvm/coproc.c | 111 ++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 84 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index c58a351..84d3ddb 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -682,17 +682,60 @@ static struct coproc_reg invariant_cp15[] = {
> { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
> };
>
> -static int reg_from_user(void *val, const void __user *uaddr, u64 id)
> +/*
> + * Reads a register value from a userspace address to a kernel u64
> + * variable. Register size must be always 8 bytes.
> + */
> +static int reg_from_user_to_u64(u64 *val, const void __user *uaddr, u64 id)
> +{
> + unsigned long regsize = KVM_REG_SIZE(id);
> +
> + if (regsize != 8)
> + return -ENOENT;
no such file?
This error check actually makes me even more convinced that we're
pushing the wrong functionality into this function, the _u64 function
should only be called if we have regsize == 8.
> +
> + if (copy_from_user(val, uaddr, regsize) != 0)
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +/*
> + * Reads a register value from a userspace address to a kernel u32 variable
> + * or a kernel array of two u32 values. That is, it may really copy two
> + * u32 registers when used with registers defined by the ABI to be 8 bytes
> + * long (e.g. TTBR0/TTBR1).
> + */
> +static int reg_from_user_to_u32(u32 *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;
> }
>
> -static int reg_to_user(void __user *uaddr, const void *val, u64 id)
> +/*
> + * Writes a register value to a userspace address from a kernel u64 value.
> + * Register size must be always 8 bytes.
> + */
> +static int reg_to_user_from_u64(void __user *uaddr, const u64 *val, u64 id)
> +{
> + unsigned long regsize = KVM_REG_SIZE(id);
> +
> + if (regsize != 8)
> + return -ENOENT;
same here
> +
> + if (copy_to_user(uaddr, val, regsize) != 0)
> + return -EFAULT;
> + return 0;
> +}
so yeah, we may not need multiple functions at all...
> +
> +/*
> + * Writes a register value to a userspace address from a kernel u32 variable
> + * or a kernel array of two u32 values. That is, it may really copy two
> + * u32 registers when used with registers defined by the ABI to be 8 bytes
> + * long (e.g. TTBR0/TTBR1).
> + */
> +static int reg_to_user_from_u32(void __user *uaddr, const u32 *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 +745,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
> {
> struct coproc_params params;
> const struct coproc_reg *r;
> + int ret = -ENOENT;
>
> if (!index_to_params(id, ¶ms))
> return -ENOENT;
> @@ -710,7 +754,13 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
> if (!r)
> return -ENOENT;
>
> - return reg_to_user(uaddr, &r->val, id);
> + if (KVM_REG_SIZE(id) == 4) {
> + u32 val = r->val;
> + ret = reg_to_user_from_u32(uaddr, &val, id);
> + } else if (KVM_REG_SIZE(id) == 8) {
> + ret = reg_to_user_from_u64(uaddr, &r->val, id);
> + }
> + return ret;
> }
>
> static int set_invariant_cp15(u64 id, void __user *uaddr)
> @@ -718,7 +768,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 +776,14 @@ 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_to_u32(&val32, uaddr, id);
> + val = val32;
> + } else if (KVM_REG_SIZE(id) == 8) {
> + err = reg_from_user_to_u64(&val, uaddr, id);
> + }
> if (err)
> return err;
>
> @@ -894,8 +951,8 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
> if (vfpid < num_fp_regs()) {
> if (KVM_REG_SIZE(id) != 8)
> return -ENOENT;
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
> - id);
> + return reg_to_user_from_u64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
> + id);
> }
>
> /* FP control registers are all 32 bit. */
> @@ -904,22 +961,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
>
> switch (vfpid) {
> case KVM_REG_ARM_VFP_FPEXC:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
> + return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
> case KVM_REG_ARM_VFP_FPSCR:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
> + return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
> case KVM_REG_ARM_VFP_FPINST:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
> + return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
> case KVM_REG_ARM_VFP_FPINST2:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
> + return reg_to_user_from_u32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
> case KVM_REG_ARM_VFP_MVFR0:
> val = fmrx(MVFR0);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user_from_u32(uaddr, &val, id);
> case KVM_REG_ARM_VFP_MVFR1:
> val = fmrx(MVFR1);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user_from_u32(uaddr, &val, id);
> case KVM_REG_ARM_VFP_FPSID:
> val = fmrx(FPSID);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user_from_u32(uaddr, &val, id);
> default:
> return -ENOENT;
> }
> @@ -938,8 +995,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
> if (vfpid < num_fp_regs()) {
> if (KVM_REG_SIZE(id) != 8)
> return -ENOENT;
> - return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
> - uaddr, id);
> + return reg_from_user_to_u64(&vcpu->arch.vfp_guest.fpregs[vfpid],
> + uaddr, id);
> }
>
> /* FP control registers are all 32 bit. */
> @@ -948,28 +1005,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
>
> switch (vfpid) {
> case KVM_REG_ARM_VFP_FPEXC:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
> + return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
> case KVM_REG_ARM_VFP_FPSCR:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
> + return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
> case KVM_REG_ARM_VFP_FPINST:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
> + return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
> case KVM_REG_ARM_VFP_FPINST2:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
> + return reg_from_user_to_u32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
> /* These are invariant. */
> case KVM_REG_ARM_VFP_MVFR0:
> - if (reg_from_user(&val, uaddr, id))
> + if (reg_from_user_to_u32(&val, uaddr, id))
> return -EFAULT;
> if (val != fmrx(MVFR0))
> return -EINVAL;
> return 0;
> case KVM_REG_ARM_VFP_MVFR1:
> - if (reg_from_user(&val, uaddr, id))
> + if (reg_from_user_to_u32(&val, uaddr, id))
> return -EFAULT;
> if (val != fmrx(MVFR1))
> return -EINVAL;
> return 0;
> case KVM_REG_ARM_VFP_FPSID:
> - if (reg_from_user(&val, uaddr, id))
> + if (reg_from_user_to_u32(&val, uaddr, id))
> return -EFAULT;
> if (val != fmrx(FPSID))
> return -EINVAL;
> @@ -1016,7 +1073,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> 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);
> + return reg_to_user_from_u32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
but is this correct? will you not end up copying the wrong words in the
wrong places now?
I think here you need to do the following if regsize == 8:
u64 val = cp15_64_read(r->reg);
reg_to_user(uaddr, &val, reg->id);
> }
>
> int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> @@ -1035,7 +1092,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> 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);
> + return reg_from_user_to_u32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
And here (again if regsize == 8):
u64 val;
reg_from_user(uaddr, &val, reg->id);
cp15_64_write(val, r->reg);
and then you can actually use the cp15_64_{read,write} functions in your
access_vm_reg() function as well.
> }
>
> static unsigned int num_demux_regs(void)
> --
> 1.8.1.4
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
