lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 19 Nov 2021 08:28:21 -0800 From: Kees Cook <keescook@...omium.org> To: LEROY Christophe <christophe.leroy@...roup.eu> Cc: Michael Ellerman <mpe@...erman.id.au>, kernel test robot <lkp@...el.com>, Benjamin Herrenschmidt <benh@...nel.crashing.org>, Paul Mackerras <paulus@...ba.org>, Sudeep Holla <sudeep.holla@....com>, Peter Zijlstra <peterz@...radead.org>, Nicholas Piggin <npiggin@...il.com>, "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>, "Eric W. Biederman" <ebiederm@...ssion.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>, "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org> Subject: Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs On Fri, Nov 19, 2021 at 08:46:27AM +0000, LEROY Christophe wrote: > > > Le 18/11/2021 à 21:36, Kees Cook a écrit : > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memset(), avoid intentionally writing across > > neighboring fields. > > > > Add a struct_group() for the spe registers so that memset() can correctly reason > > about the size: > > > > In function 'fortify_memset_chk', > > inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3: > > >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] > > 195 | __write_overflow_field(); > > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > > > Reported-by: kernel test robot <lkp@...el.com> > > Signed-off-by: Kees Cook <keescook@...omium.org> > > Reviewed-by: Christophe Leroy <christophe.leroy@...roup.eu> > > However, is it really worth adding that grouping ? Wouldn't it be > cleaner to handle evr[] and acc separately ? Now that we are using > unsafe variants of get/put user performance wouldn't be impacted. I'm fine with whatever is desired here. I reworked an earlier version of this patch based on mpe's feedback, so I can certain rework it again. :) > > I have some doubts about things like: > > unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr, > ELF_NEVRREG * sizeof(u32), failed); > > Because as far as I can see, ELF_NEVRREG is 34 but mc_vregs is a table > of 33 u32 and is at the end of the structure: > > struct mcontext { > elf_gregset_t mc_gregs; > elf_fpregset_t mc_fregs; > unsigned long mc_pad[2]; > elf_vrregset_t mc_vregs __attribute__((__aligned__(16))); > }; > > typedef elf_vrreg_t elf_vrregset_t[ELF_NVRREG]; > > # define ELF_NEVRREG 34 /* includes acc (as 2) */ > # define ELF_NVRREG 33 /* includes vscr */ I don't know these internals very well -- do you want me to change this specifically somehow? With the BUILD_BUG_ON()s added, there's no binary change here -- I wanted to make sure nothing was different in the output. -Kees > > > > > --- > > arch/powerpc/include/asm/processor.h | 6 ++++-- > > arch/powerpc/kernel/signal_32.c | 14 +++++++++----- > > 2 files changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h > > index e39bd0ff69f3..978a80308466 100644 > > --- a/arch/powerpc/include/asm/processor.h > > +++ b/arch/powerpc/include/asm/processor.h > > @@ -191,8 +191,10 @@ struct thread_struct { > > int used_vsr; /* set if process has used VSX */ > > #endif /* CONFIG_VSX */ > > #ifdef CONFIG_SPE > > - unsigned long evr[32]; /* upper 32-bits of SPE regs */ > > - u64 acc; /* Accumulator */ > > + struct_group(spe, > > + unsigned long evr[32]; /* upper 32-bits of SPE regs */ > > + u64 acc; /* Accumulator */ > > + ); > > unsigned long spefscr; /* SPE & eFP status */ > > unsigned long spefscr_last; /* SPEFSCR value on last prctl > > call or trap return */ > > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c > > index 00a9c9cd6d42..5e1664b501e4 100644 > > --- a/arch/powerpc/kernel/signal_32.c > > +++ b/arch/powerpc/kernel/signal_32.c > > @@ -527,16 +527,20 @@ static long restore_user_regs(struct pt_regs *regs, > > regs_set_return_msr(regs, regs->msr & ~(MSR_FP | MSR_FE0 | MSR_FE1)); > > > > #ifdef CONFIG_SPE > > - /* force the process to reload the spe registers from > > - current->thread when it next does spe instructions */ > > + /* > > + * Force the process to reload the spe registers from > > + * current->thread when it next does spe instructions. > > + * Since this is user ABI, we must enforce the sizing. > > + */ > > + BUILD_BUG_ON(sizeof(current->thread.spe) != ELF_NEVRREG * sizeof(u32)); > > regs_set_return_msr(regs, regs->msr & ~MSR_SPE); > > if (msr & MSR_SPE) { > > /* restore spe registers from the stack */ > > - unsafe_copy_from_user(current->thread.evr, &sr->mc_vregs, > > - ELF_NEVRREG * sizeof(u32), failed); > > + unsafe_copy_from_user(¤t->thread.spe, &sr->mc_vregs, > > + sizeof(current->thread.spe), failed); > > current->thread.used_spe = true; > > } else if (current->thread.used_spe) > > - memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32)); > > + memset(¤t->thread.spe, 0, sizeof(current->thread.spe)); > > > > /* Always get SPEFSCR back */ > > unsafe_get_user(current->thread.spefscr, (u32 __user *)&sr->mc_vregs + ELF_NEVRREG, failed); > > -- Kees Cook
Powered by blists - more mailing lists