On 4 June 2014 17:40, Victor Kamensky <victor.kamensky@xxxxxxxxxx> wrote:
> 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.
>
I don't feel strongly about it, the compiler is probably smart enough
to choose an optimal solution anyhow.
>>> + } 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.
yeah, I'm not 100% convinced that it will be cleaner with an inline
function, this does look somewhat clean, so depends on how it looks
like.
>
> And after that, I'll run a lot of testing for my current series and report
> the whole series again.
>
Awesome, thanks.
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
