Thread (1 message) 1 message, 1 author, 2022-06-09

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

From: Christophe Leroy <hidden>
Date: 2022-06-09 09:21:40

Possibly related (same subject, not in this thread)

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
quoted hunk
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/ (local)

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 <redacted>
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
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help