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 <redacted>
Signed-off-by: Kees Cook <redacted>
---
arch/powerpc/include/asm/processor.h | 6 ++++--
arch/powerpc/kernel/signal_32.c | 14 +++++++++-----
2 files changed, 13 insertions(+), 7 deletions(-)
@@ -191,8 +191,10 @@ struct thread_struct {intused_vsr;/* set if process has used VSX */#endif /* CONFIG_VSX */#ifdef CONFIG_SPE-unsignedlongevr[32];/* upper 32-bits of SPE regs */-u64acc;/* Accumulator */+struct_group(spe,+unsignedlongevr[32];/* upper 32-bits of SPE regs */+u64acc;/* Accumulator */+);unsignedlongspefscr;/* SPE & eFP status */unsignedlongspefscr_last;/* SPEFSCR value on last prctlcallortrapreturn*/
@@ -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->threadwhenitnextdoesspeinstructions*/+/*+*Forcetheprocesstoreloadthesperegistersfrom+*current->threadwhenitnextdoesspeinstructions.+*SincethisisuserABI,wemustenforcethesizing.+*/+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;}elseif(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 <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Christophe Leroy <redacted>
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 */
@@ -191,8 +191,10 @@ struct thread_struct {intused_vsr;/* set if process has used VSX */#endif /* CONFIG_VSX */#ifdef CONFIG_SPE-unsignedlongevr[32];/* upper 32-bits of SPE regs */-u64acc;/* Accumulator */+struct_group(spe,+unsignedlongevr[32];/* upper 32-bits of SPE regs */+u64acc;/* Accumulator */+);unsignedlongspefscr;/* SPE & eFP status */unsignedlongspefscr_last;/* SPEFSCR value on last prctlcallortrapreturn*/
@@ -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->threadwhenitnextdoesspeinstructions*/+/*+*Forcetheprocesstoreloadthesperegistersfrom+*current->threadwhenitnextdoesspeinstructions.+*SincethisisuserABI,wemustenforcethesizing.+*/+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;}elseif(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 :
quoted
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 <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Christophe Leroy <redacted>
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
@@ -191,8 +191,10 @@ struct thread_struct {intused_vsr;/* set if process has used VSX */#endif /* CONFIG_VSX */#ifdef CONFIG_SPE-unsignedlongevr[32];/* upper 32-bits of SPE regs */-u64acc;/* Accumulator */+struct_group(spe,+unsignedlongevr[32];/* upper 32-bits of SPE regs */+u64acc;/* Accumulator */+);unsignedlongspefscr;/* SPE & eFP status */unsignedlongspefscr_last;/* SPEFSCR value on last prctlcallortrapreturn*/
@@ -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->threadwhenitnextdoesspeinstructions*/+/*+*Forcetheprocesstoreloadthesperegistersfrom+*current->threadwhenitnextdoesspeinstructions.+*SincethisisuserABI,wemustenforcethesizing.+*/+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;}elseif(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:
quoted
Le 18/11/2021 à 21:36, Kees Cook a écrit :
quoted
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 <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Christophe Leroy <redacted>
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.
quoted
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). :)
--
Kees Cook
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-11-22 05:43:46
LEROY Christophe [off-list ref] writes:
Le 18/11/2021 à 21:36, Kees Cook a écrit :
quoted
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 <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Christophe Leroy <redacted>
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 [off-list ref] writes:
quoted
Le 18/11/2021 à 21:36, Kees Cook a écrit :
quoted
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 <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Christophe Leroy <redacted>
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-11-24 00:08:36
Kees Cook [off-list ref] writes:
On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
quoted
LEROY Christophe [off-list ref] writes:
quoted
Le 18/11/2021 à 21:36, Kees Cook a écrit :
quoted
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 <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Christophe Leroy <redacted>
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 [off-list ref] writes:
quoted
On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
quoted
LEROY Christophe [off-list ref] writes:
quoted
Le 18/11/2021 à 21:36, Kees Cook a écrit :
quoted
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 <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Christophe Leroy <redacted>
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
--
Kees Cook
From: Michael Ellerman <hidden> Date: 2021-12-07 13:28:50
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:
[...]