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 <m...@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-986939e40...@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: sta...@vger.kernel.org # v3.13+ > Reported-by: Ariel Miculas <ariel.micu...@belden.com> > Signed-off-by: Michael Ellerman <m...@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 >
