Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Mark Rutland]

On Mon, Apr 15, 2024 at 11:27:02AM +0100, Russell King (Oracle) wrote:
> On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote:
> > On 2024/04/15 18:44, Russell King (Oracle) wrote:
> > > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote:
> > >> On 2024/04/15 18:02, Mark Rutland wrote:
> > >>>   08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")
> > >>>
> > >>> That commit says that all accesses are bounce-buffered and bypass the check,
> > >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was
> > >>> wrong or the user_regset_copyin() code has changed.
> > >>
> > >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@xxxxxxxxxxxxxxxxxxx ?
> > > 
> > > Have you visited that URL? It doesn't point to an email containing a
> > > patch, so sorry, I don't know what patch you're referring to.
> > > 
> > 
> > Containing a link to a diff. ;-)
> > 
> > diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> > index c421a899fc84..347611ae762f 100644
> > --- a/arch/arm/kernel/ptrace.c
> > +++ b/arch/arm/kernel/ptrace.c
> > @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target,
> >  		   const void *kbuf, const void __user *ubuf)
> >  {
> >  	struct thread_info *thread = task_thread_info(target);
> > +	const unsigned int pos0 = pos;
> > +	char buf[sizeof(struct user_fp)];
> > +	int ret;
> >  
> > -	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> > -		&thread->fpstate,
> > -		0, sizeof(struct user_fp));
> > +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> > +				 buf, 0, sizeof(struct user_fp));
> > +	if (!ret)
> > +		memcpy(&thread->fpstate, buf, pos - pos0);
> > +	return ret;
> >  }
> >  
> >  #ifdef CONFIG_VFP
> 
> No, not unless there is really no other option. It's hacking around the
> issue, creating two copy operations of the data (one onto the stack)
> rather than solving it properly - and I will not put up with that kind
> of mentality - it's a completely broken approach to open source
> software. If there is a problem, always fix it using the correct fix,
> never try to sticky-plaster around a problem.
> 
> It seems there is a way for architectures to tell the code what is
> safe to write to, and it seems that a misunderstanding meant this
> wasn't implemented. So let's see whether it's possible to fix that
> first.

I completely agree.

We'll have to wait for Kees to wake up, but IIUC one assumption here was that
thread_info was particularly sensitive, and hence any state to be copied
to/from userspace would live in thread_struct. Either we need to remove that
assumption, or we need to move things so that we can use
arch_thread_struct_whitelist().

Given that arm always selects THREAD_INFO_IN_TASK, I think it would be a fairly
mechanical change to move fp_state (and vfp_state!) into thread_struct. That
would mean that the TI_FPSTATE offset would grow, but assuming that still fits
into an ADD immediate, we'd be ok, and then arch_thread_struct_whitelist()
could be used to handle these structures.

Mark.