On 28 May 2014 02:55, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
> 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?
What error code would you like me to use? I used ENOENT driven
by existing examples in the same file like in
static int demux_c15_set(u64 id, void __user *uaddr)
...
if (KVM_REG_SIZE(id) != 4)
return -ENOENT;
...
static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
{
...
if (KVM_REG_SIZE(id) != 8)
return -ENOENT;
> 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.
Is not above check trying to enforce exactly what you said? _u64
function will fail if it gets any other size than 8.
Not on the side I will try to rework _u32 and regsize == 8 along the
lines you suggested: read into u64 using _u64 version, and than use
something like vcpu_cp15_64_high, vcpu_cp15_64_low to put it
into array of u32. In this case I will add
if (KVM_REG_SIZE(id) != 4)
return -ENOENT;
to make sure _32 version used only with regsize == 32
>
>> +
>> + 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...
I am not sure that I follow above completely, I think we would
need to have separate _u32 and _u64 functions with proper
checks inside to enforce rule that _u32 and kernel target
type u32 is used with regsize == 4, and _u64 and kernel target
type u64 is used with regsize == 8. Otherwise if it is only one
function and it is called currently with the way we want, how
can we make sure that in future these rules will stay the same?
Or maybe we can use one macro that checks the passed
target kernel type size matches regsize. In this case code will look
nice and we would have enforcement we need. Let me try
that.
Thanks,
Victor
>> +
>> +/*
>> + * 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
