[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 at 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 at vger.kernel.org # v3.13+
> Reported-by: Ariel Miculas <ariel.miculas at belden.com>
> Signed-off-by: Michael Ellerman <mpe at 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
>