[PATCH REPOST 3/5] ARM: kvm one_reg coproc set and get BE fixes

[christoffer.dall@linaro.org (Christoffer Dall)]

Hi Victor,

On Fri, Dec 20, 2013 at 08:48:43AM -0800, Victor Kamensky wrote:
> This patch fixes issue of reading and writing

interesting line break.

an issue with

> ARM V7 registers values from/to user land. Existing code was designed to
> work only in LE case.

The existing code...

'LE case'?  'little-endian'?

> 
> struct kvm_one_reg
> ------------------
> 
> registers value passed through kvm_one_reg structure. It is used by

registers value passed through?  What are you trying to say?

> KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls. Note by looking at structure

by the KVM...

the structure

> itself we cannot tell what is size of register. Note that structure carries

the size of the register

> address of user memory, 'addr' where register should be read or written

I'm a little confused as to the value of this Section of the commit
text.  I believe the ONE_REG interface is quite well documented
already...

> 
> Setting register (from user-land to kvm)
> ----------------------------------------
> 
> kvm_arm_set_reg takes vcpu and pointer to struct kvm_one_reg which already
> read from user space

I think you could ditch this first sentence

> 
> kvm_arm_set_reg calls set_core_reg or kvm_arm_coproc_set_reg

nit: adding kvm_arm_set_reg() makes it clear that this is the function
you're refering to, and not the ioctl as a concept.

> 
> set_core_reg deals only with 4 bytes registers, it just reads 4 bytes from
> user space and store it properly into vcpu->arch.regs

stores

> 
> kvm_arm_coproc_set_reg deals with registers of different size. At certain

different sizes

At a certain point

> point code reaches phase where it retrieves description of register by id

the description of a register

> and it knows register size, which could be either 4 or 8 bytes. Kernel code

s/could be/is/

Kernel code is ready?

> is ready to read values from user space, but destination type may vary. It
> could be pointer to 32 bit integer or it could be pointer to 64 bit
> integer. And all possible permutation of size and destination pointer are

permutations

> possible.  Depending on destination pointer type, 4 bytes or 8 bytes, two

the destination pointer type

> new helper functions are introduced  - reg_from_user32 and reg_from_user64.
> They are used instead of reg_from_user function which could work only in
> LE case.

which only worked in

> 
> Size  sizeof(*DstInt)   Function used to read from user
>    4                4   reg_from_user32
>    8                4   reg_from_user32 - read two registers
>    4                8   reg_from_user64 - need special handling for BE
>    8                8   reg_from_user64
> 
> Getting register (to user-land from kvm)
> ----------------------------------------
> 
> Situation with reading registers is similar to writing. Integer pointer

The situation

> type of register to be copied could be 4 or 8 bytes. And size passed in

The integer pointer

pointer to be copied?  Please clarify what you are referring to.

> struct kvm_one_reg could be 4 or 8. And any permutation is possible.

Any permutation of source pointer type and size is possible.

> Depending on src pointer type, 4 bytes or 8 bytes, two new helper functions
> are introduced - reg_from_user32 and reg_to_user64. They are used instead

reg_to_user32?

> of reg_to_user function, which could work only in LE case.

the reg_to_user, which worked only for LE.

> 
> Size  sizeof(*SrcInt)   Function used to write to user
>    4                4   reg_to_user32
>    8                4   reg_to_user32 - writes two registers
>    4                8   reg_to_user64 - need special handleing for BE
>    8                8   reg_to_user64

I think it could be slightly more helpful to put a comment on the
functions, like "Write to 32-bit user pointer" on reg_to_user32, but
it's up to you.

> 
> Note code does assume that it can only deals with 4 or 8 byte registers.

Note: We only support register sizes of 4 or 8 bytes.

> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>

I don't mean to hammer on your commit message with all my comments.  I
really do appreciate you taking the time to document your changes.
However, with the level of detail you are providing in the commit, I
think you have to be slightly more careful with the language, so that it
doesn't end up being misleading instead of helpful.  I think you could
sum this up much shorter to simply say that core register handling is
already endian-safe, but coprocessors and vfpregs use reg_to_user which
is not endian-safe, and therefore needs changing.

The motivation about the pointer types and register sizes being
arbitrarily different is important though, so I appreciate you listing
that.


> ---
>  arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 69 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 78c0885..64b2b94 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -634,17 +634,61 @@ 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)
> +static int reg_from_user64(u64 *val, const void __user *uaddr, u64 id)
> +{
> +	unsigned long regsize = KVM_REG_SIZE(id);
> +	union {
> +		u32	word;
> +		u64	dword;
> +	} tmp = {0};
> +
> +	if (copy_from_user(&tmp, uaddr, regsize) != 0)
> +		return -EFAULT;
> +
> +	switch (regsize) {
> +	case 4:
> +		*val = tmp.word;
> +		break;
> +	case 8:
> +		*val = tmp.dword;
> +		break;
> +	}
> +	return 0;
> +}

You stated in the commit message that any permutation of
KVM_REG_SIZE(id) and sizeof(*val) is possible.

So doesn't this totally mess up the the kernel if I pass a 32-bit
pointer to reg_from_user64?  Or is that not really the case and that's
an exception to all of the permutations?

Basically you KVM_REG_SIZE(id) and sizeof your destination pointer type
should always match, but we abuse this slightly so far.  I don't think
you should cater to that, but just require callers to always provide a
consistent size/type pair (you could also add a union you use as a
parameter instead, or have two typed parameter) and simplify into a
single function.

The only special cases you now have to deal with are in:
set_invariant_cp15(): declare two temp variables of u32 and u64 sizes
get_invariant_cp15(): either have temporary values or change val in
                      corproc_reg to be a union

The current scheme is pretty hard to understand and to make sure we're
not breaking anything...

> +
> +/* Note it may really copy two u32 registers */
> +static int reg_from_user32(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)
> +static int reg_to_user64(void __user *uaddr, const u64 *val, u64 id)
> +{
> +	unsigned long regsize = KVM_REG_SIZE(id);
> +	union {
> +		u32	word;
> +		u64	dword;
> +	} tmp;
> +
> +	switch (regsize) {
> +	case 4:
> +		tmp.word = *val;
> +		break;
> +	case 8:
> +		tmp.dword = *val;
> +		break;
> +	}
> +
> +	if (copy_to_user(uaddr, &tmp, regsize) != 0)
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +/* Note it may really copy two u32 registers */
> +static int reg_to_user32(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;
> @@ -662,7 +706,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
>  	if (!r)
>  		return -ENOENT;
>  
> -	return reg_to_user(uaddr, &r->val, id);
> +	return reg_to_user64(uaddr, &r->val, id);
>  }
>  
>  static int set_invariant_cp15(u64 id, void __user *uaddr)
> @@ -678,7 +722,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
>  	if (!r)
>  		return -ENOENT;
>  
> -	err = reg_from_user(&val, uaddr, id);
> +	err = reg_from_user64(&val, uaddr, id);
>  	if (err)
>  		return err;
>  
> @@ -846,7 +890,7 @@ 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],
> +		return reg_to_user64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
>  				   id);
>  	}
>  
> @@ -856,22 +900,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_user32(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_user32(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_user32(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_user32(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_user32(uaddr, &val, id);
>  	case KVM_REG_ARM_VFP_MVFR1:
>  		val = fmrx(MVFR1);
> -		return reg_to_user(uaddr, &val, id);
> +		return reg_to_user32(uaddr, &val, id);
>  	case KVM_REG_ARM_VFP_FPSID:
>  		val = fmrx(FPSID);
> -		return reg_to_user(uaddr, &val, id);
> +		return reg_to_user32(uaddr, &val, id);
>  	default:
>  		return -ENOENT;
>  	}
> @@ -890,8 +934,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_user64(&vcpu->arch.vfp_guest.fpregs[vfpid],
> +				       uaddr, id);
>  	}
>  
>  	/* FP control registers are all 32 bit. */
> @@ -900,28 +944,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_user32(&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_user32(&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_user32(&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_user32(&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_user32(&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_user32(&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_user32(&val, uaddr, id))
>  			return -EFAULT;
>  		if (val != fmrx(FPSID))
>  			return -EINVAL;
> @@ -968,7 +1012,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_user32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
>  }
>  
>  int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> @@ -987,7 +1031,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_user32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
>  }
>  
>  static unsigned int num_demux_regs(void)
> -- 
> 1.8.1.4
> 

Thanks,
-- 
Christoffer