[PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Kees Cook]

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(-)

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(&current->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(&current->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);
-- 
2.30.2

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Kees Cook]

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(&current->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(&current->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

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Christophe Leroy]

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

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Kees Cook]

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

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Michael Ellerman]

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

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Kees Cook]

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

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Michael Ellerman]

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

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs

[Michael Ellerman]

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