Re: [PATCH 05/11] MIPS: ptrace: Always copy FCSR in FP regset

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 23 July 2014 15:12, Paul Burton <paul.burton@xxxxxxxxxx> wrote:
> On Wed, Jul 23, 2014 at 02:40:10PM +0100, Alex Smith wrote:
>> Copy FCSR in the FP regset to match the original pre-regset core dumper.
>> The code paths for where sizeof(union fpureg) == sizeof(elf_fpreg_t)
>> already do so, but they actually copy 4 bytes more than they should do
>> as FCSR is only 32 bits. The not equal code paths do not copy it at all.
>> Therefore change the copy to be done explicitly (with the correct size)
>> for both paths.
>
> Ah, I hadn't realised that ELF_NFPREG == 33, sneaky! That together with
> the "XXX fcr31" comment led me to believe the FP regset didn't include
> FCSR which is why I hadn't fixed the oops there or taken it into account
> for the case where FPR size != sizeof(elf_fpreg_t) (ie. when MSA support
> is enabled).
>
>> Additionally, clear the cause bits from FCSR when setting the FP regset
>> to avoid the possibility of causing an FP exception (and an oops) in the
>> kernel.
>>
>> Signed-off-by: Alex Smith <alex@xxxxxxxxxxxxxxxx>
>> Cc: Paul Burton <paul.burton@xxxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx> # v3.13+
>> ---
>> This patch incorporates a fix for another instance of the bug fixed by
>> Paul Burton's patch "MIPS: prevent user from setting FCSR cause bits" -
>> the code path in fpr_set for sizeof(fpureg) == sizeof(elf_fpreg_t)
>> copied fcr31 without clearing cause bits. I've incorporated a fix for
>> it into this patch to so that it's easier to apply both patches without
>> conflicts.
>> ---
>>  arch/mips/kernel/ptrace.c | 61 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 38 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
>> index 8bd13ed..ffc2e37 100644
>> --- a/arch/mips/kernel/ptrace.c
>> +++ b/arch/mips/kernel/ptrace.c
>> @@ -409,23 +409,28 @@ static int fpr_get(struct task_struct *target,
>>       int err;
>>       u64 fpr_val;
>>
>> -     /* XXX fcr31  */
>> -
>> -     if (sizeof(target->thread.fpu.fpr[i]) == sizeof(elf_fpreg_t))
>> -             return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>> -                                        &target->thread.fpu,
>> -                                        0, sizeof(elf_fpregset_t));
>> -
>> -     for (i = 0; i < NUM_FPU_REGS; i++) {
>> -             fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
>> +     if (sizeof(target->thread.fpu.fpr[i]) == sizeof(elf_fpreg_t)) {
>>               err = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>> -                                       &fpr_val, i * sizeof(elf_fpreg_t),
>> -                                       (i + 1) * sizeof(elf_fpreg_t));
>> +                                       &target->thread.fpu.fpr,
>> +                                       0, NUM_FPU_REGS * sizeof(elf_fpreg_t));
>>               if (err)
>>                       return err;
>> +     } else {
>> +             for (i = 0; i < NUM_FPU_REGS; i++) {
>> +                     fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
>> +                     err = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>> +                                               &fpr_val,
>> +                                               i * sizeof(elf_fpreg_t),
>> +                                               (i + 1) * sizeof(elf_fpreg_t));
>> +                     if (err)
>> +                             return err;
>> +             }
>>       }
>>
>> -     return 0;
>> +     return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
>> +                         &target->thread.fpu.fcr31,
>> +                         NUM_FPU_REGS * sizeof(elf_fpreg_t),
>> +                         (NUM_FPU_REGS * sizeof(elf_fpreg_t)) + sizeof(u32));
>
> The only problem I can think of is that the final register in the regset
> will still be treated as 64b (regset->size) as far as ptrace is
> concerned, so I'm not sure how best to handle this. I presume the pre
> regset core dump format placed the 32b FCSR value immediately after
> the 64b $f31, as you have here? In which case we should probably at
> least zero out the other 4 bytes of this final "register", assuming
> the extra 4 bytes compared to the pre-regset version isn't a problem?

Yes, this should now exactly match the old core dump code, which
copies the 32-bit FCSR immediately after f31 (see dump_task_fpu). The
last 4 bytes were still there with the old code, but it never actually
touched them. You're right that this should zero it out. I'll do a v2
with that fixed.

Thanks,
Alex

>
> Thanks,
>     Paul
>
>>  }
>>
>>  static int fpr_set(struct task_struct *target,
>> @@ -436,23 +441,33 @@ static int fpr_set(struct task_struct *target,
>>       unsigned i;
>>       int err;
>>       u64 fpr_val;
>> +     u32 fcr31;
>>
>> -     /* XXX fcr31  */
>> -
>> -     if (sizeof(target->thread.fpu.fpr[i]) == sizeof(elf_fpreg_t))
>> -             return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
>> -                                       &target->thread.fpu,
>> -                                       0, sizeof(elf_fpregset_t));
>> -
>> -     for (i = 0; i < NUM_FPU_REGS; i++) {
>> +     if (sizeof(target->thread.fpu.fpr[i]) == sizeof(elf_fpreg_t)) {
>>               err = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
>> -                                      &fpr_val, i * sizeof(elf_fpreg_t),
>> -                                      (i + 1) * sizeof(elf_fpreg_t));
>> +                                      &target->thread.fpu.fpr,
>> +                                      0, NUM_FPU_REGS * sizeof(elf_fpreg_t));
>>               if (err)
>>                       return err;
>> -             set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
>> +     } else {
>> +             for (i = 0; i < NUM_FPU_REGS; i++) {
>> +                     err = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
>> +                                              &fpr_val,
>> +                                              i * sizeof(elf_fpreg_t),
>> +                                              (i + 1) * sizeof(elf_fpreg_t));
>> +                     if (err)
>> +                             return err;
>> +                     set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
>> +             }
>>       }
>>
>> +     err = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &fcr31,
>> +                         NUM_FPU_REGS * sizeof(elf_fpreg_t),
>> +                         (NUM_FPU_REGS * sizeof(elf_fpreg_t)) + sizeof(u32));
>> +     if (err)
>> +             return err;
>> +
>> +     target->thread.fpu.fcr31 = fcr31 & ~FPU_CSR_ALL_X;
>>       return 0;
>>  }
>>
>> --
>> 1.9.1
>>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]