PowerPC arch_ptrace() writes beyond thread_struct/task_struct

PowerPC arch_ptrace() writes beyond thread_struct/task_struct

[Radu Rendec]

Hi Everyone,

I'm seeing some weird memory corruption that I have been able to isolate
to arch_ptrace() [arch/powerpc/kernel/ptrace.c] and PTRACE_POKEUSR. I am
on PowerPC 32 (MPC8378), kernel 4.9.179.

It's not very easy for me to test on the latest kernel, but I guess
little has changed since 4.9 in either the architecture specific ptrace
code or PowerPC register data structures.

What happens is that gdb calls ptrace(PTRACE_POKEUSER) with addr=0x158.
This goes down to arch_ptrace() [arch/powerpc/kernel/ptrace.c], inside
`case PTRACE_POKEUSR`, on the branch that does this:

    memcpy(&child->thread.TS_FPR(fpidx), &data,
            sizeof(long));

where:
    index = addr >> 2 = 0x56 = 86
    fpidx = index - PT_FPR0 = 86 - 48 = 38
    &child->thread.TS_FPR(fpidx) = (void *)child + 1296

    offsetof(struct task_struct, thread) = 960
    sizeof(struct thread_struct) = 336
    sizeof(struct task_struct) = 1296

In other words, the memcpy() call writes just beyond thread_struct
(which is also beyond task_struct, for that matter).

This should never get past the bounds checks for `index`, so perhaps
there is a mismatch between ptrace macros and the actual register data
structures layout.

I will continue to investigate, but I'm not familiar with the PowerPC
registers so it will take a while before I make sense of all the data
structures and macros. Hopefully this rings a bell to someone who is
already familiar with those and could figure out quickly what the
problem is.

Best regards,
Radu Rendec

Re: PowerPC arch_ptrace() writes beyond thread_struct/task_struct

[Christophe Leroy]

Le 05/06/2019 à 23:45, Radu Rendec a écrit :
> Hi Everyone,
> 
> I'm seeing some weird memory corruption that I have been able to isolate
> to arch_ptrace() [arch/powerpc/kernel/ptrace.c] and PTRACE_POKEUSR. I am
> on PowerPC 32 (MPC8378), kernel 4.9.179.
> 
> It's not very easy for me to test on the latest kernel, but I guess
> little has changed since 4.9 in either the architecture specific ptrace
> code or PowerPC register data structures.
> 
> What happens is that gdb calls ptrace(PTRACE_POKEUSER) with addr=0x158.
> This goes down to arch_ptrace() [arch/powerpc/kernel/ptrace.c], inside
> `case PTRACE_POKEUSR`, on the branch that does this:
> 
>      memcpy(&child->thread.TS_FPR(fpidx), &data,
>              sizeof(long));
> 
> where:
>      index = addr >> 2 = 0x56 = 86
>      fpidx = index - PT_FPR0 = 86 - 48 = 38

In struct thread_fp_state, fpr field is u64, so I guess we should have 
the following on PPC32:

fpidx = (index - PT_FPR0) >> 1;

Christophe

>      &child->thread.TS_FPR(fpidx) = (void *)child + 1296
> 
>      offsetof(struct task_struct, thread) = 960
>      sizeof(struct thread_struct) = 336
>      sizeof(struct task_struct) = 1296
> 
> In other words, the memcpy() call writes just beyond thread_struct
> (which is also beyond task_struct, for that matter).
> 
> This should never get past the bounds checks for `index`, so perhaps
> there is a mismatch between ptrace macros and the actual register data
> structures layout.
> 
> I will continue to investigate, but I'm not familiar with the PowerPC
> registers so it will take a while before I make sense of all the data
> structures and macros. Hopefully this rings a bell to someone who is
> already familiar with those and could figure out quickly what the
> problem is.
> 
> Best regards,
> Radu Rendec
> 

Re: PowerPC arch_ptrace() writes beyond thread_struct/task_struct

[Radu Rendec]

On Thu, 2019-06-06 at 07:15 +0200, Christophe Leroy wrote:
> 
> Le 05/06/2019 à 23:45, Radu Rendec a écrit :
> > Hi Everyone,
> > 
> > I'm seeing some weird memory corruption that I have been able to isolate
> > to arch_ptrace() [arch/powerpc/kernel/ptrace.c] and PTRACE_POKEUSR. I am
> > on PowerPC 32 (MPC8378), kernel 4.9.179.
> > 
> > It's not very easy for me to test on the latest kernel, but I guess
> > little has changed since 4.9 in either the architecture specific ptrace
> > code or PowerPC register data structures.
> > 
> > What happens is that gdb calls ptrace(PTRACE_POKEUSER) with addr=0x158.
> > This goes down to arch_ptrace() [arch/powerpc/kernel/ptrace.c], inside
> > `case PTRACE_POKEUSR`, on the branch that does this:
> > 
> >      memcpy(&child->thread.TS_FPR(fpidx), &data,
> >              sizeof(long));
> > 
> > where:
> >      index = addr >> 2 = 0x56 = 86
> >      fpidx = index - PT_FPR0 = 86 - 48 = 38
> 
> In struct thread_fp_state, fpr field is u64, so I guess we should have 
> the following on PPC32:
> 
> fpidx = (index - PT_FPR0) >> 1;

I guess this would only apply to PPC32, since everything up to fpidx is
calculated in units of sizeof(long) - which is 4 on PPC32 and 8 on
PPC64. But fpr[0:31] is always u64.

It also looks odd that only sizeof(long) bytes are ever copied for any
given fpr[fpidx], which means one half of the u64 is never accessible on
PPC32.

Ont other thing I don't get is the "+1" in the definition of PT_FPSCR
for PPC32:

    #define PT_FPSCR (PT_FPR0 + 2*32 + 1)

Looking at struct thread_fp_state, fpscr follows immediately after
fpr[31]. Is the FPSCR register only 32-bit on PPC32? Is it stored in the
2nd half of (struct thread_fp_state).fpscr? This line:

    child->thread.fp_state.fpscr = data;

suggests so. And in that case, the "+1" in PT_FPSCR makes sense, but
only for big endian: assigning `data` (which is "long", 32-bit) to the
`fpscr` field (which is "u64") would go to the higher address, which is
indeed "+1" in units of 32-bit words.

Then there is also a problem is the condition that determines whether
memcpy() is used to access one of the fpr[0:31] or fpscr is assigned
directly:

    if (fpidx < (PT_FPSCR - PT_FPR0))

The case when the supplied addr points to the lower half of fpscr (which
is unused on PPC32?) erroneously indexes into fpr[0:31].

Is there any documentation of what "addr" is supposed to mean?


> >      &child->thread.TS_FPR(fpidx) = (void *)child + 1296
> > 
> >      offsetof(struct task_struct, thread) = 960
> >      sizeof(struct thread_struct) = 336
> >      sizeof(struct task_struct) = 1296
> > 
> > In other words, the memcpy() call writes just beyond thread_struct
> > (which is also beyond task_struct, for that matter).
> > 
> > This should never get past the bounds checks for `index`, so perhaps
> > there is a mismatch between ptrace macros and the actual register data
> > structures layout.
> > 
> > I will continue to investigate, but I'm not familiar with the PowerPC
> > registers so it will take a while before I make sense of all the data
> > structures and macros. Hopefully this rings a bell to someone who is
> > already familiar with those and could figure out quickly what the
> > problem is.