| Message ID | m2k568xlfg.fsf@igel.home (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> The ptrace compat wrapper mishandles access to the fpu registers. The > PTRACE_PEEKUSR and PTRACE_POKEUSR requests miscalculate the index into > the fpr array due to the broken FPINDEX macro. The > PPC_PTRACE_PEEKUSR_3264 request needs to use the same formula that the > native ptrace interface uses when operating on the register number (as > opposed to the 4-byte offset). The PPC_PTRACE_POKEUSR_3264 request > didn't take TS_FPRWIDTH into account. > > This was tested with the gdb testsuite on a G5. So if you're looking fixing 32 bit apps ptracing 64 bit apps, does that mean we can get a single 32 bit GDB that'll ptrace both 64 and 32 bit apps? I'd been looking for a ptrace test suite... thanks! > Signed-off-by: Andreas Schwab <schwab@linux-m68k.org> > > --- > diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c > index 197d49c..f992eaf 100644 > --- a/arch/powerpc/kernel/ptrace32.c > +++ b/arch/powerpc/kernel/ptrace32.c > @@ -67,7 +67,7 @@ static long compat_ptrace_old(struct task_struct *child, lo ng request, > /* Macros to workout the correct index for the FPR in the thread struct */ > #define FPRNUMBER(i) (((i) - PT_FPR0) >> 1) > #define FPRHALF(i) (((i) - PT_FPR0) & 1) > -#define FPRINDEX(i) TS_FPRWIDTH * FPRNUMBER(i) + FPRHALF(i) > +#define FPRINDEX(i) TS_FPRWIDTH * FPRNUMBER(i) * 2 + FPRHALF(i) ACK, I have the same patch here: http://patchwork.ozlabs.org/patch/24940/ > > long compat_arch_ptrace(struct task_struct *child, compat_long_t request, > compat_ulong_t caddr, compat_ulong_t cdata) > @@ -169,7 +169,7 @@ long compat_arch_ptrace(struct task_struct *child, compat _long_t request, > if (numReg >= PT_FPR0) { > flush_fp_to_thread(child); > tmp = ((unsigned long int *)child->thread.fpr) > - [FPRINDEX(numReg)]; > + [TS_FPRWIDTH * (numReg - PT_FPR0)]; > } else { /* register within PT_REGS struct */ > tmp = ptrace_get_reg(child, numReg); > } > @@ -263,7 +263,8 @@ long compat_arch_ptrace(struct task_struct *child, compat _long_t request, > ret = ptrace_put_reg(child, numReg, freg); > } else { > flush_fp_to_thread(child); > - ((unsigned int *)child->thread.regs)[index] = data; > + ((unsigned int *)child->thread.regs) > + [FPRINDEX(index)] = data; This index is into the ptregs structure not the fpr. I'm not sure the FPRINDEX macro is applicable here. Mikey > ret = 0; > } > break; > > -- > Andreas Schwab, schwab@linux-m68k.org > GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 > "And now for something completely different." > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@ozlabs.org > https://ozlabs.org/mailman/listinfo/linuxppc-dev >
Michael Neuling <mikey@neuling.org> writes: > So if you're looking fixing 32 bit apps ptracing 64 bit apps, does that > mean we can get a single 32 bit GDB that'll ptrace both 64 and 32 bit > apps? Currently gdb only supports 32x64 debugging for the SPU. >> @@ -263,7 +263,8 @@ long compat_arch_ptrace(struct task_struct *child, compat > _long_t request, >> ret = ptrace_put_reg(child, numReg, freg); >> } else { >> flush_fp_to_thread(child); >> - ((unsigned int *)child->thread.regs)[index] = data; >> + ((unsigned int *)child->thread.regs) >> + [FPRINDEX(index)] = data; > > This index is into the ptregs structure not the fpr. I'm not sure the > FPRINDEX macro is applicable here. You're right, this hunk is bogus. But indexing off thread.regs is totally bogus as well. I think what was intented is this: @@ -263,7 +263,9 @@ long compat_arch_ptrace(struct task_stru ret = ptrace_put_reg(child, numReg, freg); } else { flush_fp_to_thread(child); - ((unsigned int *)child->thread.regs)[index] = data; + ((unsigned int *)child->thread.fpr) + [TS_FPRWIDTH * (numReg - PT_FPR0) * 2 + + index % 2] = data; ret = 0; } break; But gdb does not actually use PPC_PTRACE_POKEUSR_3264. Andreas.
> > So if you're looking fixing 32 bit apps ptracing 64 bit apps, does that > > mean we can get a single 32 bit GDB that'll ptrace both 64 and 32 bit > > apps? > > Currently gdb only supports 32x64 debugging for the SPU. Ok, thanks. > >> @@ -263,7 +263,8 @@ long compat_arch_ptrace(struct task_struct *child, com pat > > _long_t request, > >> ret = ptrace_put_reg(child, numReg, freg); > >> } else { > >> flush_fp_to_thread(child); > >> - ((unsigned int *)child->thread.regs)[index] = data; > >> + ((unsigned int *)child->thread.regs) > >> + [FPRINDEX(index)] = data; > > > > This index is into the ptregs structure not the fpr. I'm not sure the > > FPRINDEX macro is applicable here. > > You're right, this hunk is bogus. But indexing off thread.regs is > totally bogus as well. I think what was intented is this: > > @@ -263,7 +263,9 @@ long compat_arch_ptrace(struct task_stru > ret = ptrace_put_reg(child, numReg, freg); > } else { > flush_fp_to_thread(child); > - ((unsigned int *)child->thread.regs)[index] = data; > + ((unsigned int *)child->thread.fpr) > + [TS_FPRWIDTH * (numReg - PT_FPR0) * 2 + > + index % 2] = data; I think the indexing here should be the same as PEEKUSR_3264. This looks better but all this magic indexing makes me want to vomit. I'd like to fix this stuff but I've been avoiding it since we don't have a decent test case/suite to make sure it's not bust. Mikey > ret = 0; > } > break; > > But gdb does not actually use PPC_PTRACE_POKEUSR_3264. > > Andreas. > > -- > Andreas Schwab, schwab@linux-m68k.org > GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 > "And now for something completely different." >
Michael Neuling <mikey@neuling.org> writes: >> @@ -263,7 +263,9 @@ long compat_arch_ptrace(struct task_stru >> ret = ptrace_put_reg(child, numReg, freg); >> } else { >> flush_fp_to_thread(child); >> - ((unsigned int *)child->thread.regs)[index] = data; >> + ((unsigned int *)child->thread.fpr) >> + [TS_FPRWIDTH * (numReg - PT_FPR0) * 2 + >> + index % 2] = data; > > I think the indexing here should be the same as PEEKUSR_3264. This > looks better but all this magic indexing makes me want to vomit. How about this instead: @@ -241,6 +241,7 @@ long compat_arch_ptrace(struct task_stru case PPC_PTRACE_POKEUSR_3264: { u32 index; u32 numReg; + u32 *tmp; ret = -EIO; /* Determine which register the user wants */ @@ -263,7 +264,8 @@ long compat_arch_ptrace(struct task_stru ret = ptrace_put_reg(child, numReg, freg); } else { flush_fp_to_thread(child); - ((unsigned int *)child->thread.regs)[index] = data; + tmp = (u32 *)child->thread.fpr[numReg - PT_FPR0]; + tmp[index % 2] = data; ret = 0; } break; Andreas.
In message <m2myb32rk8.fsf@igel.home> you wrote: > Michael Neuling <mikey@neuling.org> writes: > > >> @@ -263,7 +263,9 @@ long compat_arch_ptrace(struct task_stru > >> ret = ptrace_put_reg(child, numReg, freg); > >> } else { > >> flush_fp_to_thread(child); > >> - ((unsigned int *)child->thread.regs)[index] = data; > >> + ((unsigned int *)child->thread.fpr) > >> + [TS_FPRWIDTH * (numReg - PT_FPR0) * 2 + > >> + index % 2] = data; > > > > I think the indexing here should be the same as PEEKUSR_3264. This > > looks better but all this magic indexing makes me want to vomit. > > How about this instead: > > @@ -241,6 +241,7 @@ long compat_arch_ptrace(struct task_stru > case PPC_PTRACE_POKEUSR_3264: { > u32 index; > u32 numReg; > + u32 *tmp; > > ret = -EIO; > /* Determine which register the user wants */ > @@ -263,7 +264,8 @@ long compat_arch_ptrace(struct task_stru > ret = ptrace_put_reg(child, numReg, freg); > } else { > flush_fp_to_thread(child); > - ((unsigned int *)child->thread.regs)[index] = data; > + tmp = (u32 *)child->thread.fpr[numReg - PT_FPR0]; > + tmp[index % 2] = data; I do like this approach better (two arrays) but there is no accounting for TS_WIDTH, so I'm not sure it works. We *really* need a test case for this stuff :-) Mikey > ret = 0; > } > break; > > Andreas. > > -- > Andreas Schwab, schwab@linux-m68k.org > GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 > "And now for something completely different." >
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c index 197d49c..f992eaf 100644 --- a/arch/powerpc/kernel/ptrace32.c +++ b/arch/powerpc/kernel/ptrace32.c @@ -67,7 +67,7 @@ static long compat_ptrace_old(struct task_struct *child, long request, /* Macros to workout the correct index for the FPR in the thread struct */ #define FPRNUMBER(i) (((i) - PT_FPR0) >> 1) #define FPRHALF(i) (((i) - PT_FPR0) & 1) -#define FPRINDEX(i) TS_FPRWIDTH * FPRNUMBER(i) + FPRHALF(i) +#define FPRINDEX(i) TS_FPRWIDTH * FPRNUMBER(i) * 2 + FPRHALF(i) long compat_arch_ptrace(struct task_struct *child, compat_long_t request, compat_ulong_t caddr, compat_ulong_t cdata) @@ -169,7 +169,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request, if (numReg >= PT_FPR0) { flush_fp_to_thread(child); tmp = ((unsigned long int *)child->thread.fpr) - [FPRINDEX(numReg)]; + [TS_FPRWIDTH * (numReg - PT_FPR0)]; } else { /* register within PT_REGS struct */ tmp = ptrace_get_reg(child, numReg); } @@ -263,7 +263,8 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request, ret = ptrace_put_reg(child, numReg, freg); } else { flush_fp_to_thread(child); - ((unsigned int *)child->thread.regs)[index] = data; + ((unsigned int *)child->thread.regs) + [FPRINDEX(index)] = data; ret = 0; } break;
The ptrace compat wrapper mishandles access to the fpu registers. The PTRACE_PEEKUSR and PTRACE_POKEUSR requests miscalculate the index into the fpr array due to the broken FPINDEX macro. The PPC_PTRACE_PEEKUSR_3264 request needs to use the same formula that the native ptrace interface uses when operating on the register number (as opposed to the 4-byte offset). The PPC_PTRACE_POKEUSR_3264 request didn't take TS_FPRWIDTH into account. This was tested with the gdb testsuite on a G5. Signed-off-by: Andreas Schwab <schwab@linux-m68k.org> ---