linux-kernel - Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZhztQ9CvDm3UPgE9@FVFF77S0Q05N>
Date: Mon, 15 Apr 2024 10:02:59 +0100
From: Mark Rutland <mark.rutland@....com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
	Kees Cook <keescook@...omium.org>
Cc: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	Linux ARM <linux-arm-kernel@...ts.infradead.org>,
	syzbot <syzbot+cb76c2983557a07cdb14@...kaller.appspotmail.com>,
	linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

On Wed, Apr 03, 2024 at 05:12:07PM +0100, Russell King (Oracle) wrote:
> On Tue, Mar 05, 2024 at 08:27:07PM +0900, Tetsuo Handa wrote:
> > Hello.
> > 
> > syzbot is reporting kernel memory overwrite attempt at fpa_set().
> > I guessed that the amount to copy from/to should be sizeof(union fp_state)
> > than sizeof(struct user_fp), for arch_ptrace(PTRACE_[SG]ETFPREGS) for arm
> > is using offset == 0 and size == sizeof(union fp_state). But my guess did not
> > solve the issue ( https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000 ).
> 
> This is silly.
> 
> sizeof(struct user_fp) is:
> 
> 	8 * (
> 		1 bit for sign1, 15 bits unused => 2 bytes
> 		1 bit for sign2, 14 bits unused, 1 bit for j => 2 bytes
> 		31 bits for mantissa1 => 4 bytes
> 		32 bits for mantissa0 => 4 bytes
> 	) +
> 	4 bytes for fpsr
> 	4 bytes for fpcr
> 	8 bytes for ftype
> 	4 bytes for init_flag
> 
> This totals 8 * 12 + 4 + 4 + 8 + 4 = 116 bytes or 29 32-bit quantities,
> or 29 "unsigned int"s.
> 
> This is copied into union fp_state. This union is made up of one of
> several different formats depending on the FP being used. user_fp
> doesn't reflect this. However, one of these, struct fp_soft_struct,
> is specifically sized to ensure that user_fp is _smaller_.
> 
> struct fp_soft_struct is 35 unsigned int's. This is 140 bytes. This
> is larger than sizeof(user_fp).
> 
> Therefore, there is _no way_ for fpa_set() to overwrite anything
> outside of thread_info->fpstate, because sizeof(struct user_fp)
> is smaller than sizeof(thread->fpstate).
> 
> Syzbot appears to be wrong in this instance.

I believe the problem here is that HARDENED_USERCOPY tries to prevent any
usercopy to/from task_struct except for fields that are explicitly whitelisted
(which all need to be in one contiguous range). That was added in commit:

  5905429ad85657c2 ("fork: Provide usercopy whitelisting for task_struct")

However, architectures only have the option to provide
arch_thread_struct_whitelist() to whitelist some fields in thread_struct, not
thread_info where the fp_state lives. On arm arch_thread_struct_whitelist()
whitelists precisely nothing:

  static inline void arch_thread_struct_whitelist(unsigned long *offset,
                                                 unsigned long *size)
  {
         *offset = *size = 0;
  }

.. which was added in commit:

  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.

Kees, I believe you need to look at this.

See the dashboard page at:

  https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14

Mark.