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

[Mark Rutland]

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.