Re: [PATCH 0/1] PPC32: fix ptrace() access to FPU registers

[Radu Rendec <radu.rendec@gmail.com>]

On Fri, 2021-06-11 at 10:37 -0400, Radu Rendec wrote:
>On Fri, 2021-06-11 at 08:02 +0200, Christophe Leroy wrote:
>>Le 19/06/2019 à 14:57, Radu Rendec a écrit :
>>> On Wed, 2019-06-19 at 10:36 +1000, Daniel Axtens wrote:
>>>> Andreas Schwab <
>>>> schwab@linux-m68k.org
>>>>> writes:
>>>>
>>>>> On Jun 18 2019, Radu Rendec <
>>>>> radu.rendec@gmail.com
>>>>>> wrote:
>>>>>
>>>>>> Since you already have a working setup, it would be nice if you could
>>>>>> add a printk to arch_ptrace() to print the address and confirm what I
>>>>>> believe happens (by reading the gdb source code).
>>>>>
>>>>> A ppc32 ptrace syscall goes through compat_arch_ptrace.
>>>
>>> Right. I completely overlooked that part.
>>>
>>>> Ah right, and that (in ptrace32.c) contains code that will work:
>>>>
>>>>
>>>> 			/*
>>>> 			 * the user space code considers the floating point
>>>> 			 * to be an array of unsigned int (32 bits) - the
>>>> 			 * index passed in is based on this assumption.
>>>> 			 */
>>>> 			tmp = ((unsigned int *)child->thread.fp_state.fpr)
>>>> 				[FPRINDEX(index)];
>>>>
>>>> FPRINDEX is defined above to deal with the various manipulations you
>>>> need to do.
>>>
>>> Correct. Basically it does the same that I did in my patch: it divides
>>> the index again by 2 (it's already divided by 4 in compat_arch_ptrace()
>>> so it ends up divided by 8), then takes the least significant bit and
>>> adds it to the index. I take bit 2 of the original address, which is the
>>> same thing (because in FPRHALF() the address is already divided by 4).
>>>
>>> So we have this in ptrace32.c:
>>>
>>> #define FPRNUMBER(i) (((i) - PT_FPR0) >> 1)
>>> #define FPRHALF(i) (((i) - PT_FPR0) & 1)
>>> #define FPRINDEX(i) TS_FPRWIDTH * FPRNUMBER(i) * 2 + FPRHALF(i)
>>>
>>> index = (unsigned long) addr >> 2;
>>> (unsigned int *)child->thread.fp_state.fpr)[FPRINDEX(index)]
>>>
>>>
>>> And we have this in my patch:
>>>
>>> fpidx = (addr - PT_FPR0 * sizeof(long)) / 8;
>>> (void *)&child->thread.TS_FPR(fpidx) + (addr & 4)
>>>
>>>> Radu: I think we want to copy that working code back into ptrace.c.
>>>
>>> I'm not sure that would work. There's a subtle difference: the code in
>>> ptrace32.c is always compiled on a 64-bit kernel and the user space
>>> calling it is always 32-bit; on the other hand, the code in ptrace.c can
>>> be compiled on either a 64-bit kernel or a 32-bit kernel and the user
>>> space calling it always has the same "bitness" as the kernel.
>>>
>>> One difference is the size of the CPU registers. On 64-bit they are 8
>>> byte long and user space knows that and generates 8-byte aligned
>>> addresses. So you have to divide the address by 8 to calculate the CPU
>>> register index correctly, which compat_arch_ptrace() currently doesn't.
>>>
>>> Another difference is that on 64-bit `long` is 8 bytes, so user space
>>> can read a whole FPU register in a single ptrace call.
>>>
>>> Now that we are all aware of compat_arch_ptrace() (which handles the
>>> special case of a 32-bit process running on a 64-bit kernel) I would say
>>> the patch is correct and does the right thing for both 32-bit and 64-bit
>>> kernels and processes.
>>>
>>>> The challenge will be unpicking the awful mess of ifdefs in ptrace.c
>>>> and making it somewhat more comprehensible.
>>>
>>> I'm not sure what ifdefs you're thinking about. The only that are used
>>> inside arch_ptrace() are PT_FPR0, PT_FPSCR and TS_FPR, which seem to be
>>> correct.
>>>
>>> But perhaps it would be useful to change my patch and add a comment just
>>> before arch_ptrace() that explains how the math is done and that the
>>> code must work on both 32-bit and 64-bit, the user space address
>>> assumptions, etc.
>>>
>>> By the way, I'm not sure the code in compat_arch_ptrace() handles
>>> PT_FPSCR correctly. It might (just because fpscr is right next to fpr[]
>>> in memory - and that's a hack), but I can't figure out if it accesses
>>> the right half.
>>>
>>
>>Does the issue still exists ? If yes, the patch has to be rebased.
>
>Hard to say. I'm still using 4.9 (stable) on the systems that I created
>the patch for. I tried to rebase, and the patch no longer applies. It
>looks like there have been some changes around that area, notably your
>commit e009fa433542, so it could actually be fixed now.
>
>It's been exactly two years since I sent the patch and I don't remember
>all the details. I will have to go back and look. Also, running a recent
>kernel on my PPC32 systems is not an option because there are a bunch of
>custom patches that would have to be ported. I will try in a VM and get
>back to you, hopefully early next week.

I finally had time to test everything properly. I can now confirm that
the original problem no longer exists, so the patch doesn't need to be
rebased.

I tested all three variants: 32-bit program on 32-bit kernel, 32-bit
program on 64-bit kernel and 64-bit program on 64-bit kernel.

Best regards,
Radu