Subject: Re: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

From: Gabriel Paubert <[email-masked]>
Date: Mon, 1 Feb 2021 17:54:40 +0100

On Mon, Feb 01, 2021 at 09:55:44AM -0600, Christopher M. Riedl wrote:
> On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote:
> > From: Christopher M. Riedl
> > > Sent: 28 January 2021 04:04
> > > 
> > > Reuse the "safe" implementation from signal.c except for calling
> > > unsafe_copy_from_user() to copy into a local buffer.
> > > 
> > > Signed-off-by: Christopher M. Riedl <[email-masked]>
> > > ---
> > >  arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > > 
> > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> > > index 2559a681536e..c18402d625f1 100644
> > > --- a/arch/powerpc/kernel/signal.h
> > > +++ b/arch/powerpc/kernel/signal.h
> > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > >  				&buf[i], label);\
> > >  } while (0)
> > > 
> > > +#define unsafe_copy_fpr_from_user(task, from, label)	do {		\
> > > +	struct task_struct *__t = task;					\
> > > +	u64 __user *__f = (u64 __user *)from;				\
> > > +	u64 buf[ELF_NFPREG];						\
> >
> > How big is that buffer?
> > Isn't is likely to be reasonably large compared to a reasonable
> > kernel stack frame.
> > Especially since this isn't even a leaf function.
> >
> 
> I think Christophe answered this - I don't really have an opinion either
> way. What would be a 'reasonable' kernel stack frame for reference?

See include/linux/poll.h, where the limit is of the order of 800 bytes
and the number of entries in an on stack array is chosen at compile time
(different between 32 and 64 bit for example).

The values are used in do_sys_poll, which, with almost 1000 bytes of
stack footprint, appears close to the top of "make checkstack". In
addition do_sys_poll has to call the ->poll function of every file
descriptor in its table, so it is not a tail function.

This 264 bytes array looks reasonable, but please use 'make checkstack'
to verify that the function's total stack usage stays within reason.

	Gabriel

> 
> > > +	int i;								\
> > > +									\
> > > +	unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double),	\
> >
> > That really ought to be sizeof(buf).
> >
> 
> Agreed, I will fix this. Thanks!
> 
> > David
> >
> >
> > > +				label);					\
> > > +	for (i = 0; i < ELF_NFPREG - 1; i++)				\
> > > +		__t->thread.TS_FPR(i) = buf[i];				\
> > > +	__t->thread.fp_state.fpscr = buf[i];				\
> > > +} while (0)
> > > +
> > > +#define unsafe_copy_vsx_from_user(task, from, label)	do {		\
> > > +	struct task_struct *__t = task;					\
> > > +	u64 __user *__f = (u64 __user *)from;				\
> > > +	u64 buf[ELF_NVSRHALFREG];					\
> > > +	int i;								\
> > > +									\
> > > +	unsafe_copy_from_user(buf, __f,					\
> > > +				ELF_NVSRHALFREG * sizeof(double),	\
> > > +				label);					\
> > > +	for (i = 0; i < ELF_NVSRHALFREG ; i++)				\
> > > +		__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];	\
> > > +} while (0)
> > > +
> > > +
> > >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > >  #define unsafe_copy_ckfpr_to_user(to, task, label)	do {		\
> > >  	struct task_struct *__t = task;					\
> > > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > >  	unsafe_copy_to_user(to, (task)->thread.fp_state.fpr,	\
> > >  			    ELF_NFPREG * sizeof(double), label)
> > > 
> > > +#define unsafe_copy_fpr_from_user(task, from, label)			\
> > > +	unsafe_copy_from_user((task)->thread.fp_state.fpr, from,	\
> > > +			    ELF_NFPREG * sizeof(double), label)
> > > +
> > >  static inline unsigned long
> > >  copy_fpr_to_user(void __user *to, struct task_struct *task)
> > >  {
> > > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
> > >  #else
> > >  #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
> > > 
> > > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0)
> > > +
> > >  static inline unsigned long
> > >  copy_fpr_to_user(void __user *to, struct task_struct *task)
> > >  {
> > > --
> > > 2.26.1
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> > MK1 1PT, UK
> > Registration No: 1397386 (Wales)
>