[PATCH v3 13/14] ARM64: KVM: set and get of sys registers in BE case

[Victor Kamensky]

On 14 May 2014 01:45, Marc Zyngier <marc.zyngier at arm.com> wrote:
> Hi Victor,
>
> On Tue, May 13 2014 at  5:14:05 pm BST, Victor Kamensky <victor.kamensky at linaro.org> wrote:
>> This patch addresses issue of reading and writing V8 sys registers in
>> BE case. Since only register size function deals with is 8 bytes,
>> existing code works in both little and big endian cases.
>> Removed comment about little endian. Added BUG_ON that register
>> size should be always 8 bytes.
>>
>> If these functions would ever need to support both 8 bytes and 4 bytes
>> register sizes to deals with them in endian agnostic way code should
>> do something along these lines:
>>
>>        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;
>>        }
>>
>> Signed-off-by: Victor Kamensky <victor.kamensky at linaro.org>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 19 +++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 0324458..060c3a9 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -776,18 +776,25 @@ static struct sys_reg_desc invariant_sys_regs[] = {
>>         NULL, get_ctr_el0 },
>>  };
>>
>> -static int reg_from_user(void *val, const void __user *uaddr, u64 id)
>> +static int reg_from_user(u64 *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)
>> +     unsigned long regsize = KVM_REG_SIZE(id);
>> +
>> +     BUG_ON(regsize != 8);
>
> I haven't had time to review this series just yet, but this bit just
> sends chivers down my spine.
>
> regsize is derived from id, which comes from a struct one_reg, which is
> directly provided by userspace. Here, you're trusting the luser to give
> you 8 as a size, and panic the kernel if not.
>
> As much as I'd like to qualify this as only being a slightly undesirable
> effect, I think it deserves a NAK.

Fair enough. I agree. Good catch! I was following on Christoffer's comments
at [1], but I have not thought it through. Please advise should I come back to
previous version as in [2] or just ignore any sizes other than 8 without
having BUG_ON?

Thanks,
Victor

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/241815.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/231891.html

> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny.