[PATCH] Fix ptrace compat wrapper for fpu register access

[PATCH] Fix ptrace compat wrapper for fpu register access

[Andreas Schwab]

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>

---
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;

-- 
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."

Re: [PATCH] Fix ptrace compat wrapper for fpu register access

[Michael Neuling]

> 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
> 

Re: [PATCH] Fix ptrace compat wrapper for fpu register access

[Andreas Schwab]

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.

-- 
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."

Re: [PATCH] Fix ptrace compat wrapper for fpu register access

[Michael Neuling]

> > 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."
> 

Re: [PATCH] Fix ptrace compat wrapper for fpu register access

[Andreas Schwab]

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.

-- 
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."

Re: [PATCH] Fix ptrace compat wrapper for fpu register access

[Michael Neuling]

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."
> 

Re: [PATCH] Fix ptrace compat wrapper for fpu register access

[Michael Neuling]

> > >> @@ -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] = d
ata;
> > >> +			((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.

Ok, here's another stab at trying to fix this and clean it up.  

Mikey

powerpc: Fix ptrace compat wrapper for fpu register access

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.

Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kernel/ptrace32.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Index: linux-2.6-ozlabs/arch/powerpc/kernel/ptrace32.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/ptrace32.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/ptrace32.c
@@ -67,7 +67,8 @@ static long compat_ptrace_old(struct tas
 /* 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)
+#define FPRINDEX_3264(i) (TS_FPRWIDTH * ((i) - PT_FPR0))
 
 long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 			compat_ulong_t caddr, compat_ulong_t cdata)
@@ -168,8 +169,9 @@ long compat_arch_ptrace(struct task_stru
 		CHECK_FULL_REGS(child->thread.regs);
 		if (numReg >= PT_FPR0) {
 			flush_fp_to_thread(child);
-			tmp = ((unsigned long int *)child->thread.fpr)
-				[FPRINDEX(numReg)];
+			// get 64 bit FPR
+			tmp = ((u64 *)child->thread.fpr)
+				[FPRINDEX_3264(numReg)];
 		} else { /* register within PT_REGS struct */
 			tmp = ptrace_get_reg(child, numReg);
 		} 
@@ -262,8 +264,13 @@ long compat_arch_ptrace(struct task_stru
 				freg = (freg & 0xfffffffful) | (data << 32);
 			ret = ptrace_put_reg(child, numReg, freg);
 		} else {
+			u64 *tmp;
 			flush_fp_to_thread(child);
-			((unsigned int *)child->thread.regs)[index] = data;
+			// get 64 bit FPR ...
+			tmp = &(((u64 *)child->thread.fpr)
+				[FPRINDEX_3264(numReg)]);
+			// ... write the 32 bit part we want
+			((u32 *)tmp)[index % 2] = data;
 			ret = 0;
 		}
 		break;

Re: [PATCH] Fix ptrace compat wrapper for fpu register access

[Michael Neuling]

In message <8394.1239000531@neuling.org> you wrote:
> > > >> @@ -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] = d
> ata;
> > > >> +			((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.
> 
> Ok, here's another stab at trying to fix this and clean it up.  

... and now without the c++ style comments...

Mikey

powerpc: Fix ptrace compat wrapper for fpu register access

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.

Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kernel/ptrace32.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Index: linux-2.6-ozlabs/arch/powerpc/kernel/ptrace32.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/ptrace32.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/ptrace32.c
@@ -67,7 +67,8 @@ static long compat_ptrace_old(struct tas
 /* 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)
+#define FPRINDEX_3264(i) (TS_FPRWIDTH * ((i) - PT_FPR0))
 
 long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 			compat_ulong_t caddr, compat_ulong_t cdata)
@@ -168,8 +169,9 @@ long compat_arch_ptrace(struct task_stru
 		CHECK_FULL_REGS(child->thread.regs);
 		if (numReg >= PT_FPR0) {
 			flush_fp_to_thread(child);
-			tmp = ((unsigned long int *)child->thread.fpr)
-				[FPRINDEX(numReg)];
+			/* get 64 bit FPR */
+			tmp = ((u64 *)child->thread.fpr)
+				[FPRINDEX_3264(numReg)];
 		} else { /* register within PT_REGS struct */
 			tmp = ptrace_get_reg(child, numReg);
 		} 
@@ -262,8 +264,13 @@ long compat_arch_ptrace(struct task_stru
 				freg = (freg & 0xfffffffful) | (data << 32);
 			ret = ptrace_put_reg(child, numReg, freg);
 		} else {
+			u64 *tmp;
 			flush_fp_to_thread(child);
-			((unsigned int *)child->thread.regs)[index] = data;
+			/* get 64 bit FPR ... */
+			tmp = &(((u64 *)child->thread.fpr)
+				[FPRINDEX_3264(numReg)]);
+			/* ... write the 32 bit part we want */
+			((u32 *)tmp)[index % 2] = data;
 			ret = 0;
 		}
 		break;