| Message ID | 20211118203604.1288379-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc/signal32: Use struct_group() to zero spe regs | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
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@intel.com> > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.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 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 */ > --- > 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); >
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@intel.com> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.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); > >
Le 19/11/2021 à 17:28, Kees Cook a écrit : > 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@intel.com> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >> >> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.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. :) Well, with oddities like the below, it may not be straight forward. If the objective is to enable FORTIFY_SOURCE, maybe that's good enough. Let see if Michael has any opinion. > >> >> 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. > Neither do I. I was just scared by what I saw while reviewing your patch. A cleanup is probably required but it can be another patch. Christophe
On Fri, Nov 19, 2021 at 05:35:00PM +0100, Christophe Leroy wrote: > Neither do I. I was just scared by what I saw while reviewing your patch. A > cleanup is probably required but it can be another patch. Heh, understood! For my end, my objective with the fortify work is to either split cross-member memcpy() calls (which is usually undesirable) or add a struct group so it can be seen as a "single member" memcpy again (and usually results in 0 differences in binary output). :)
LEROY Christophe <christophe.leroy@csgroup.eu> writes: > 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@intel.com> >> Signed-off-by: Kees Cook <keescook@chromium.org> > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> Acked-by: Michael Ellerman <mpe@ellerman.id.au> > 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. Yeah I agree we should be able to do less of these multi-field copies now that we have unsafe get/put user. But I think that's an issue for another patch, Kees' patch is an improvement, even if the code could be improved further in future. Though TBH I'm not sure what the future of SPE support is. Both GCC and glibc have dropped support for it, more than 2 years ago, so it's not clear to me if we should continue to support it in the kernel much longer. cheers
On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote: > LEROY Christophe <christophe.leroy@csgroup.eu> writes: > > 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@intel.com> > >> Signed-off-by: Kees Cook <keescook@chromium.org> > > > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > Acked-by: Michael Ellerman <mpe@ellerman.id.au> Thanks! Should I take this via my tree, or do you want to take it via ppc? -Kees
Kees Cook <keescook@chromium.org> writes: > On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote: >> LEROY Christophe <christophe.leroy@csgroup.eu> writes: >> > 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@intel.com> >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> > >> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> >> Acked-by: Michael Ellerman <mpe@ellerman.id.au> > > Thanks! Should I take this via my tree, or do you want to take it via > ppc? I don't mind. If it's easier for you to take it as part of an existing series then do that, otherwise I can pick it up. cheers
On Wed, Nov 24, 2021 at 11:08:25AM +1100, Michael Ellerman wrote: > Kees Cook <keescook@chromium.org> writes: > > On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote: > >> LEROY Christophe <christophe.leroy@csgroup.eu> writes: > >> > 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@intel.com> > >> >> Signed-off-by: Kees Cook <keescook@chromium.org> > >> > > >> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >> > >> Acked-by: Michael Ellerman <mpe@ellerman.id.au> > > > > Thanks! Should I take this via my tree, or do you want to take it via > > ppc? > > I don't mind. If it's easier for you to take it as part of an existing > series then do that, otherwise I can pick it up. Most of the other patches are going via other maintainers, so I'd love it if ppc could snag this one too. :) Thanks! -Kees
On Thu, 18 Nov 2021 12:36:04 -0800, Kees Cook wrote: > 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: > > [...] Applied to powerpc/next. [1/1] powerpc/signal32: Use struct_group() to zero spe regs https://git.kernel.org/powerpc/c/62ea67e31981bca95ec16c37e2a1fba68f3dd8c5 cheers
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);
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@intel.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- arch/powerpc/include/asm/processor.h | 6 ++++-- arch/powerpc/kernel/signal_32.c | 14 +++++++++----- 2 files changed, 13 insertions(+), 7 deletions(-)