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

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Kees Cook <keescook@chromium.org>,
	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 11:27:02 +0100	[thread overview]
Message-ID: <Zh0A9p/l8lmIrunk@shell.armlinux.org.uk> (raw)
In-Reply-To: <2ab55fd1-7eb0-488e-93ea-660fa05ee43a@I-love.SAKURA.ne.jp>

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!