Message-ID: <202111190824.AEBBE1328@keescook>
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(&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

Powered by blists - more mailing lists