On Wed, May 28, 2014 at 11:05: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 we will enforce rule that reg_from_user
> to reg_to_user can copy register value only to kernel variable with size that
> matches register size. For this purpose change functions to macros that check
> sizeof(*val) against 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 | 95 +++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 74 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index c58a351..5d45be5 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -682,26 +682,43 @@ 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)
> -{
> - /* This Just Works because we are little endian. */
> - if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
> - return -EFAULT;
> - return 0;
> -}
> +/*
> + * Reads a register value from a userspace address to a kernel
> + * variable. Note register size must match sizeof(*__val).
> + */
> +#define reg_from_user(__val, __uaddr, __id) ({ \
> + int __ret = 0; \
> + unsigned long __regsize = KVM_REG_SIZE(__id); \
> + if (sizeof(*(__val)) != __regsize) { \
> + __ret = -ENOENT; \
> + } else { \
> + if (copy_from_user((__val), (__uaddr), __regsize) != 0) \
> + __ret = -EFAULT; \
> + } \
> + __ret; \
> +})
>
> -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;
> -}
> +/*
> + * Writes a register value to a userspace address from a kernel variable.
> + * Note register size must match sizeof(*__val).
> + */
> +#define reg_to_user(__uaddr, __val, __id) ({ \
> + int __ret = 0; \
> + unsigned long __regsize = KVM_REG_SIZE(__id); \
> + if (sizeof(*(__val)) != __regsize) { \
> + __ret = -ENOENT; \
> + } else { \
> + if (copy_to_user((__uaddr), (__val), __regsize) != 0) \
> + __ret = -EFAULT; \
> + } \
> + __ret; \
> +})
Ahh, no. Please don't. If you really think we want checks that we know
how to program, then do the _u32 and _u64 versions and have a BUG_ON()
in there if the KVM_REG_SIZE does not match.
I don't think this is needed though, because the special (oddball as you
call them) cases are so rare.
>
> 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 +727,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 +742,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 +750,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;
> + } else if (KVM_REG_SIZE(id) == 8) {
> + err = reg_from_user(&val, uaddr, id);
> + }
> if (err)
> return err;
>
> @@ -1004,6 +1036,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 +1048,24 @@ 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 */
> + memcpy(&val, &(vcpu->arch.cp15[r->reg]), sizeof(val));
Are you not creating a wrongly ordered u64 on BE systems here? Surely
the least significant word will be at the lowest address.
I think you need a function to copy cp15_64_to_u64() thingy which is
taking BE/LE into consideration. You had this for one of the other
patches that you may be able to reuse.
I commented on this in the last revision too, am I getting it wrong?
> + 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 +1077,18 @@ 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)
> + memcpy(&(vcpu->arch.cp15[r->reg]), &val, sizeof(val));
same as above.
> + } 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
>
Otherwise, we are certainly moving in the right direction.
Thanks!
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
