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.
>> + } 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.
And after that, I'll run a lot of testing for my current series and report
the whole series again.
Thanks,
Victor
> Thanks,
> -Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
