| Message ID | 20220609095235.37863-1-ariel.miculas@belden.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/ptrace: Fix buffer overflow when handling PTRACE_PEEKUSER and PTRACE_POKEUSER | expand |
Le 09/06/2022 à 11:52, Ariel Miculas a écrit : > This fixes the gdbserver issue on PPC32 described here: > Link: https://linuxppc-dev.ozlabs.narkive.com/C46DRek4/debug-problems-on-ppc-83xx-target-due-to-changed-struct-task-struct > > On PPC32, 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. > > fp_state is a matrix consisting of 32 lines > /* FP and VSX 0-31 register set / > struct thread_fp_state { > u64 fpr[32][TS_FPRWIDTH] attribute((aligned(16))); > u64 fpscr; / Floating point status */ > }; > > On PPC32, PT_FPSCR is defined as: (PT_FPR0 + 2*32 + 1) > > This means the fpr index validation allows a range from 0 to 65, leading > to out-of-bounds array access. This ends up corrupting > threads_struct->state, which holds the state of the task. Thus, threads > incorrectly transition from a running state to a traced state and get > stuck in that state. > > On PPC32 it's ok to assume that TS_FPRWIDTH is 1 because CONFIG_VSX is > PPC64 specific. TS_FPROFFSET can be safely ignored, thus the assumption > that fpr is an array of 32 elements of type u64 holds true. > > Solution taken from arch/powerpc/kernel/ptrace32.c > > Signed-off-by: Ariel Miculas <ariel.miculas@belden.com> > --- > arch/powerpc/kernel/ptrace/ptrace-fpu.c | 31 +++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/ptrace/ptrace-fpu.c b/arch/powerpc/kernel/ptrace/ptrace-fpu.c > index 5dca19361316..93695abbbdfb 100644 > --- a/arch/powerpc/kernel/ptrace/ptrace-fpu.c > +++ b/arch/powerpc/kernel/ptrace/ptrace-fpu.c > @@ -6,9 +6,16 @@ > > #include "ptrace-decl.h" > > +#ifdef CONFIG_PPC32 > +/* 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) * 2 + FPRHALF(i) > +#endif I can't see the benefit of such macros if they are only for PPC32. > + > int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data) > { > -#ifdef CONFIG_PPC_FPU_REGS > +#if defined(CONFIG_PPC_FPU_REGS) && !defined(CONFIG_PPC32) > unsigned int fpidx = index - PT_FPR0; > #endif #ifdefs should be avoided as much as possible. > > @@ -17,10 +24,20 @@ int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data) > > #ifdef CONFIG_PPC_FPU_REGS > flush_fp_to_thread(child); > +#ifdef CONFIG_PPC32 Here you could use IS_ENABLED(CONFIG_PPC32), it would also avoid the above #ifdef. > + /* > + * 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. > + */ > + *data = ((unsigned int *)child->thread.fp_state.fpr) > + [FPRINDEX(index)]; if I understand FPRINDEX(index) correctly, at the end we have FPRINDEX(i) == i, so I can't see the point. Michael's patch seems easier to understand. I think if one day we want something common to ppc32 and ppc64, we need to use a new macro similar to TS_FPR() but that properly takes ppc32 into account. Pay attention to not change TS_FPR() as it is used in other places where it is valid for both PPC32 and PPC64. > +#else > if (fpidx < (PT_FPSCR - PT_FPR0)) > memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long)); > else > *data = child->thread.fp_state.fpscr; > +#endif > #else > *data = 0; > #endif > @@ -30,7 +47,7 @@ int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data) > > int ptrace_put_fpr(struct task_struct *child, int index, unsigned long data) > { > -#ifdef CONFIG_PPC_FPU_REGS > +#if defined(CONFIG_PPC_FPU_REGS) && !defined(CONFIG_PPC32) > unsigned int fpidx = index - PT_FPR0; > #endif > > @@ -39,10 +56,20 @@ int ptrace_put_fpr(struct task_struct *child, int index, unsigned long data) > > #ifdef CONFIG_PPC_FPU_REGS > flush_fp_to_thread(child); > +#ifdef CONFIG_PPC32 > + /* > + * 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. > + */ > + ((unsigned int *)child->thread.fp_state.fpr) > + [FPRINDEX(index)] = data; > +#else > if (fpidx < (PT_FPSCR - PT_FPR0)) > memcpy(&child->thread.TS_FPR(fpidx), &data, sizeof(long)); > else > child->thread.fp_state.fpscr = data; > +#endif > #endif > > return 0;
diff --git a/arch/powerpc/kernel/ptrace/ptrace-fpu.c b/arch/powerpc/kernel/ptrace/ptrace-fpu.c index 5dca19361316..93695abbbdfb 100644 --- a/arch/powerpc/kernel/ptrace/ptrace-fpu.c +++ b/arch/powerpc/kernel/ptrace/ptrace-fpu.c @@ -6,9 +6,16 @@ #include "ptrace-decl.h" +#ifdef CONFIG_PPC32 +/* 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) * 2 + FPRHALF(i) +#endif + int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data) { -#ifdef CONFIG_PPC_FPU_REGS +#if defined(CONFIG_PPC_FPU_REGS) && !defined(CONFIG_PPC32) unsigned int fpidx = index - PT_FPR0; #endif @@ -17,10 +24,20 @@ int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data) #ifdef CONFIG_PPC_FPU_REGS flush_fp_to_thread(child); +#ifdef CONFIG_PPC32 + /* + * 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. + */ + *data = ((unsigned int *)child->thread.fp_state.fpr) + [FPRINDEX(index)]; +#else if (fpidx < (PT_FPSCR - PT_FPR0)) memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long)); else *data = child->thread.fp_state.fpscr; +#endif #else *data = 0; #endif @@ -30,7 +47,7 @@ int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data) int ptrace_put_fpr(struct task_struct *child, int index, unsigned long data) { -#ifdef CONFIG_PPC_FPU_REGS +#if defined(CONFIG_PPC_FPU_REGS) && !defined(CONFIG_PPC32) unsigned int fpidx = index - PT_FPR0; #endif @@ -39,10 +56,20 @@ int ptrace_put_fpr(struct task_struct *child, int index, unsigned long data) #ifdef CONFIG_PPC_FPU_REGS flush_fp_to_thread(child); +#ifdef CONFIG_PPC32 + /* + * 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. + */ + ((unsigned int *)child->thread.fp_state.fpr) + [FPRINDEX(index)] = data; +#else if (fpidx < (PT_FPSCR - PT_FPR0)) memcpy(&child->thread.TS_FPR(fpidx), &data, sizeof(long)); else child->thread.fp_state.fpscr = data; +#endif #endif return 0;
This fixes the gdbserver issue on PPC32 described here: Link: https://linuxppc-dev.ozlabs.narkive.com/C46DRek4/debug-problems-on-ppc-83xx-target-due-to-changed-struct-task-struct On PPC32, 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. fp_state is a matrix consisting of 32 lines /* FP and VSX 0-31 register set / struct thread_fp_state { u64 fpr[32][TS_FPRWIDTH] attribute((aligned(16))); u64 fpscr; / Floating point status */ }; On PPC32, PT_FPSCR is defined as: (PT_FPR0 + 2*32 + 1) This means the fpr index validation allows a range from 0 to 65, leading to out-of-bounds array access. This ends up corrupting threads_struct->state, which holds the state of the task. Thus, threads incorrectly transition from a running state to a traced state and get stuck in that state. On PPC32 it's ok to assume that TS_FPRWIDTH is 1 because CONFIG_VSX is PPC64 specific. TS_FPROFFSET can be safely ignored, thus the assumption that fpr is an array of 32 elements of type u64 holds true. Solution taken from arch/powerpc/kernel/ptrace32.c Signed-off-by: Ariel Miculas <ariel.miculas@belden.com> --- arch/powerpc/kernel/ptrace/ptrace-fpu.c | 31 +++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)