Radu Rendec <radu.ren...@gmail.com> writes:
> On Mon, 2019-06-17 at 11:19 +1000, Daniel Axtens wrote:
>> Radu Rendec <
>> radu.ren...@gmail.com
>> > writes:
>>
>> > Hi Everyone,
>> >
>> > I'm following up on the ptrace() problem that I reported a few days ago.
>> > I believe my version of the code handles all cases correctly. While the
>> > problem essentially boils down to dividing the fpidx by 2 on PPC32, it
>> > becomes tricky when the same code must work correctly on both PPC32 and
>> > PPC64.
>> >
>> > One other thing that I believe was handled incorrectly in the previous
>> > version is the unused half of fpscr on PPC32. Note that while PT_FPSCR
>> > is defined as (PT_FPR0 + 2*32 + 1), making only the upper half visible,
>> > PT_FPR0 + 2*32 still corresponds to a possible address that userspace
>> > can pass. In that case, comparing fpidx to (PT_FPSCR - PT_FPR0) would
>> > cause an invalid access to the FPU registers array.
>> >
>> > I tested the patch on 4.9.179, but that part of the code is identical in
>> > recent kernels so it should work just the same.
>> >
>> > I wrote a simple test program than can be used to quickly test (on an
>> > x86_64 host) that all cases are handled correctly for both PPC32/PPC64.
>> > The code is included below.
>> >
>> > I also tested with gdbserver (test patch included below) and verified
>> > that it generates two ptrace() calls for each FPU register, with
>> > addresses between 0xc0 and 0x1bc.
>>
>> Thanks for looking in to this. I can confirm your issue. What I'm
>> currently wondering is: what is the behaviour with a 32-bit userspace on
>> a 64-bit kernel? Should they also be going down the 32-bit path as far
>> as calculating offsets goes?
>
> Thanks for reviewing this. I haven't thought about the 32-bit userspace
> on a 64-bit kernel, that is a very good question. Userspace passes a
> pointer, so in theory it could go down either path as long as the
> pointer points to the right data type.
>
> I will go back to the gdb source code and try to figure out if that case
> is handled in a special way. If not, it's probably safe to assume that a
> 32-bit userspace should always go down the 32-bit path regardless of the
> kernel bitness (in which case I think I have to change my patch).
It doesn't seem to reproduce on a 64-bit kernel with 32-bit
userspace. Couldn't tell you why - if you can figure it out from the gdb
source code I'd love to know! I have, however, tried it - and all the fp
registers look correct and KASAN doesn't pick up any memory corruption.
Regards,
Daniel
>
> Best regards,
> Radu
>
>> > 8<--------------- Makefile ---------------------------------------------
>> > .PHONY: all clean
>> >
>> > all: ptrace-fpregs-32 ptrace-fpregs-64
>> >
>> > ptrace-fpregs-32: ptrace-fpregs.c
>> > $(CC) -o ptrace-fpregs-32 -Wall -O2 -m32 ptrace-fpregs.c
>> >
>> > ptrace-fpregs-64: ptrace-fpregs.c
>> > $(CC) -o ptrace-fpregs-64 -Wall -O2 ptrace-fpregs.c
>> >
>> > clean:
>> > rm -f ptrace-fpregs-32 ptrace-fpregs-64
>> > 8<--------------- ptrace-fpregs.c --------------------------------------
>> > #include <stdio.h>
>> > #include <errno.h>
>> >
>> > #define PT_FPR0 48
>> >
>> > #ifndef __x86_64
>> >
>> > #define PT_FPR31 (PT_FPR0 + 2*31)
>> > #define PT_FPSCR (PT_FPR0 + 2*32 + 1)
>> >
>> > #else
>> >
>> > #define PT_FPSCR (PT_FPR0 + 32)
>> >
>> > #endif
>> >
>> > int test_access(unsigned long addr)
>> > {
>> > int ret;
>> >
>> > do {
>> > unsigned long index, fpidx;
>> >
>> > ret = -EIO;
>> >
>> > /* convert to index and check */
>> > index = addr / sizeof(long);
>> > if ((addr & (sizeof(long) - 1)) || (index > PT_FPSCR))
>> > break;
>> >
>> > if (index < PT_FPR0) {
>> > ret = printf("ptrace_put_reg(%lu)", index);
>> > break;
>> > }
>> >
>> > ret = 0;
>> > #ifndef __x86_64
>> > if (index == PT_FPSCR - 1) {
>> > /* corner case for PPC32; do nothing */
>> > printf("corner_case");
>> > break;
>> > }
>> > #endif
>> > if (index == PT_FPSCR) {
>> > printf("fpscr");
>> > break;
>> > }
>> >
>> > /*
>> > * FPR is always 64-bit; on PPC32, userspace does two 32-bit
>> > * accesses. Add bit2 to allow accessing the upper half on
>> > * 32-bit; on 64-bit, bit2 is always 0 (we validate it above).
>> > */
>> > fpidx = (addr - PT_FPR0 * sizeof(long)) / 8;
>> > printf("TS_FPR[%lu] + %lu", fpidx, addr & 4);
>> > break;
>> > } while (0);
>> >
>> > return ret;
>> > }
>> >
>> > int main(void)
>> > {
>> > unsigned long addr;
>> > int rc;
>> >
>> > for (addr = 0; addr < PT_FPSCR * sizeof(long) + 16; addr++) {
>> > printf("0x%04lx: ", addr);
>> > rc = test_access(addr);
>> > if (rc < 0)
>> > printf("!err!");
>> > printf("\t<%d>\n", rc);
>> > }
>> >
>> > return 0;
>> > }
>> > 8<--------------- gdb.patch --------------------------------------------
>> > --- gdb/gdbserver/linux-low.c.orig 2019-06-10 11:45:53.810882669 -0400
>> > +++ gdb/gdbserver/linux-low.c 2019-06-10 11:49:32.272929766 -0400
>> > @@ -4262,6 +4262,8 @@ store_register (struct regcache *regcach
>> > pid = lwpid_of (get_thread_lwp (current_inferior));
>> > for (i = 0; i < size; i += sizeof (PTRACE_XFER_TYPE))
>> > {
>> > + printf("writing register #%d offset %d at address %#x\n",
>> > + regno, i, (unsigned int)regaddr);
>> > errno = 0;
>> > ptrace (PTRACE_POKEUSER, pid,
>> > /* Coerce to a uintptr_t first to avoid potential gcc warning
>> > 8<----------------------------------------------------------------------
>> >
>> > Radu Rendec (1):
>> > PPC32: fix ptrace() access to FPU registers
>> >
>> > arch/powerpc/kernel/ptrace.c | 85 ++++++++++++++++++++++--------------
>> > 1 file changed, 52 insertions(+), 33 deletions(-)
>> >
>> > --
>> > 2.20.1
