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

From: Kees Cook <keescook@chromium.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: "Russell King (Oracle)" <linux@armlinux.org.uk>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	syzbot <syzbot+cb76c2983557a07cdb14@syzkaller.appspotmail.com>,
	linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set
Date: Mon, 15 Apr 2024 10:02:42 -0700	[thread overview]
Message-ID: <202404150945.0E96AB727@keescook> (raw)
In-Reply-To: <Zh0S_7Pit6Zc04mS@FVFF77S0Q05N.cambridge.arm.com>

On Mon, Apr 15, 2024 at 12:43:59PM +0100, Mark Rutland wrote:
> 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@I-love.SAKURA.ne.jp ?
> > > > 
> > > > 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.

FWIW, the bound buffer approach is used in other places as well
(especially for places that are not fastpath, like this case), as it
creates an explicit bounds check (i.e. the buffer on the stack). But
yes, if there is a way to specifically allow this during the kmem setup,
let's do that.

The timeline on this is interesting, it first showed up on syzbot about
a year ago (2023/05/05 12:53)
https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14

But hardened usercopy (and commit 08626a6056aad824) are from Jan 2018.
No one noticed for 5 years? :P

> 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.

Sure, or use a bounce buffer, since no one noticed this for 5 years
it probably isn't heavy used[1]? But sure, I'm open to whatever --
the point is to not arbitrarily allow usercopy to touch the memory here.

-Kees

[1] We probably can't remove PTRACE_SETFPREGS, but yeah, it's really
uncommon:
https://codesearch.debian.net/search?q=ptrace%28PTRACE_SETFPREGS%2C&literal=1&perpkg=1
- systemtap test suite uses it
- rr is x86 only
- crui doesn't have a arm backend
- python ptrace may use it
- edb-debugger is x86 only
- llvm mentions it in comments only

-- 
Kees Cook