Re: [PATCH] powerpc/ptrace: Fix buffer overflow when handling PTRACE_PEEKUSER and PTRACE_POKEUSER

Re: [PATCH] powerpc/ptrace: Fix buffer overflow when handling PTRACE_PEEKUSER and PTRACE_POKEUSER

[Christophe Leroy]

Hi Michael,

Le 06/06/2022 à 16:45, Michael Ellerman a écrit :
> Hi Ariel,
> 
> I've added Christophe to Cc who works on ppc32.

I've added powerpc list

> 
> I haven't actually reproduced the crash with gdbserver, but I have a
> test case which shows the bug, so I've been able to confirm it and
> test a fix.
> 
> Thanks for your patch, but I wanted to fix it differently. Can you try
> the patch below and make sure it fixes the bug for you?
> 
> I've also attached the test case I've been using.
> 
> Christophe are you able to test these on some 32-bit machines? I've
> tested it in qemu and on one 32-bit machine I have here, but some more
> real testing would be good.

Yes I will test it but my CPUs have no FPU so it will be with the kernel 
software Math emulation. But it should make no difference I guess ?

Christophe

> 
> If the patch works then I'll need to do manual back ports for several of
> the stable kernels, and then once those are ready I will publish the
> patch.
> 
> cheers
> 
> 
> ---8<---
>  From eaa9a32fe38d8722d2da8773965309365805d66d Mon Sep 17 00:00:00 2001
> From: Michael Ellerman <mpe@ellerman.id.au>
> Date: Tue, 7 Jun 2022 00:34:56 +1000
> Subject: [PATCH] powerpc/32: Fix overread/overwrite of thread_struct via
>   ptrace
> 
> The ptrace PEEKUSR/POKEUSR (aka PEEKUSER/POKEUSER) API allows a process
> to read/write registers of another process.
> 
> To get/set a register, the API takes an index into an imaginary address
> space called the "USER area", where the registers of the process are
> laid out in some fashion.
> 
> The kernel then maps that index to a particular register in its own data
> structures and gets/sets the value.
> 
> The API only allows a single machine-word to be read/written at a time.
> So 4 bytes on 32-bit kernels and 8 bytes on 64-bit kernels.
> 
> The way floating point registers (FPRs) are addressed is somewhat
> complicated, because double precision float values are 64-bit even on
> 32-bit CPUs. That means on 32-bit kernels each FPR occupies two
> word-sized locations in the USER area. On 64-bit kernels each FPR
> occupies one word-sized location in the USER area.
> 
> Internally the kernel stores the FPRs in an array of u64s, or if VSX is
> enabled, an array of pairs of u64s where one half of each pair stores
> the FPR. Which half of the pair stores the FPR depends on the kernel's
> endianness.
> 
> To handle the different layouts of the FPRs depending on VSX/no-VSX and
> big/little endian, the TS_FPR() macro was introduced.
> 
> Unfortunately the TS_FPR() macro does not take into account the fact
> that the addressing of each FPR differs between 32-bit and 64-bit
> kernels. It just takes the index into the "USER area" passed from
> userspace and indexes into the fp_state.fpr array.
> 
> On 32-bit there are 64 indexes that address FPRs, but only 32 entries in
> the fp_state.fpr array, meaning the user can read/write 256 bytes past
> the end of the array. Because the fp_state sits in the middle of the
> thread_struct there are various fields than can be overwritten,
> including some pointers. As such it is probably exploitable.
> 
> It has also been observed to cause systems to hang or otherwise
> misbehave when using gdbserver, and is probably the root cause of this
> report which could not be easily reproduced:
>    https://lore.kernel.org/linuxppc-dev/dc38afe9-6b78-f3f5-666b-986939e40fc6@keymile.com/
> 
> Rather than trying to make the TS_FPR() macro even more complicated to
> fix the bug, or add more macros, instead add a special-case for 32-bit
> kernels. This is more obvious and hopefully avoids a similar bug
> happening again in future. Note that because 32-bit kernels never have
> VSX enabled the code doesn't need to consider TS_FPRWIDTH/OFFSET at all.
> 
> Fixes: 87fec0514f61 ("powerpc: PTRACE_PEEKUSR/PTRACE_POKEUSER of FPR registers in little endian builds")
> Cc: stable@vger.kernel.org # v3.13+
> Reported-by: Ariel Miculas <ariel.miculas@belden.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/kernel/ptrace/ptrace-fpu.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-fpu.c b/arch/powerpc/kernel/ptrace/ptrace-fpu.c
> index 5dca19361316..f406436a0f6c 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-fpu.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-fpu.c
> @@ -17,9 +17,13 @@ int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data)
>   
>   #ifdef CONFIG_PPC_FPU_REGS
>   	flush_fp_to_thread(child);
> -	if (fpidx < (PT_FPSCR - PT_FPR0))
> -		memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long));
> -	else
> +	if (fpidx < (PT_FPSCR - PT_FPR0)) {
> +		if (IS_ENABLED(CONFIG_PPC32))
> +			// The 32-bit ptrace API addresses the FPRs as 32-bit words
> +			*data = ((u32 *)child->thread.fp_state.fpr)[fpidx];
> +		else
> +			memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long));
> +	} else
>   		*data = child->thread.fp_state.fpscr;
>   #else
>   	*data = 0;
> @@ -39,9 +43,13 @@ int ptrace_put_fpr(struct task_struct *child, int index, unsigned long data)
>   
>   #ifdef CONFIG_PPC_FPU_REGS
>   	flush_fp_to_thread(child);
> -	if (fpidx < (PT_FPSCR - PT_FPR0))
> -		memcpy(&child->thread.TS_FPR(fpidx), &data, sizeof(long));
> -	else
> +	if (fpidx < (PT_FPSCR - PT_FPR0)) {
> +		if (IS_ENABLED(CONFIG_PPC32))
> +			// The 32-bit ptrace API addresses the FPRs as 32-bit words
> +			((u32 *)child->thread.fp_state.fpr)[fpidx] = data;
> +		else
> +			memcpy(&child->thread.TS_FPR(fpidx), &data, sizeof(long));
> +	} else
>   		child->thread.fp_state.fpscr = data;
>   #endif
>   

[PATCH] powerpc/ptrace: Fix buffer overflow when handling PTRACE_PEEKUSER and PTRACE_POKEUSER

[Ariel Miculas]

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

Re: [PATCH] powerpc/ptrace: Fix buffer overflow when handling PTRACE_PEEKUSER and PTRACE_POKEUSER

[Christophe Leroy]

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;