Re: hardened_usercopy 32-bit (was: Re: [tip: x86/merge] x86/fpu: Make task_struct::thread constant size)

[Kees Cook <kees@kernel.org>]

On Mon, May 05, 2025 at 01:31:59PM +0200, Ingo Molnar wrote:
> 
> * Kees Cook <kees@kernel.org> wrote:
> 
> > But as reported above, there *is* a copy in copy_uabi_to_xstate(). (It
> > seems there are several, actually.)
> > 
> > int copy_sigframe_from_user_to_xstate(struct task_struct *tsk,
> >                                       const void __user *ubuf)
> > {
> >         return copy_uabi_to_xstate(x86_task_fpu(tsk)->fpstate, NULL, ubuf, &tsk->thread.pkru);
> > }
> > 
> > This appears to be writing into x86_task_fpu(tsk)->fpstate. With or
> > without CONFIG_X86_DEBUG_FPU, this resolves to:
> > 
> > 	((struct fpu *)((void *)(task) + sizeof(*(task))))
> > 
> > i.e. the memory "after task_struct" is cast to "struct fpu", and the
> > uses the "fpstate" pointer. How that pointer gets set looks to be
> > variable, but I think the one we care about here is:
> > 
> >         fpu->fpstate = &fpu->__fpstate;
> > 
> > And struct fpu::__fpstate says:
> > 
> >         struct fpstate                  __fpstate;
> >         /*
> >          * WARNING: '__fpstate' is dynamically-sized.  Do not put
> >          * anything after it here.
> >          */
> > 
> > So we're still dealing with a dynamically sized thing, even if it's not
> > within the literal struct task_struct -- it's still in the kmem cache,
> > though.
> 
> Indeed!
> 
> > So, this is still copying out of the kmem cache for task_struct, and the
> > window seems unchanged (still fpu regs). This is what the window was
> > before:
> > 
> > void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
> > {
> >         *offset = offsetof(struct thread_struct, fpu.__fpstate.regs);
> >         *size = fpu_kernel_cfg.default_size;
> > }
> > 
> > And the same commit I mentioned above removed it.
> > 
> > I think the misunderstanding is here:
> > 
> > >    The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
> > >    now that the FPU structure is not embedded in the task struct anymore, which
> > >    reduces text footprint a bit.
> > 
> > Yes, FPU is no longer in task_struct, but it IS in the kmem cache named
> > "task_struct", since the fpstate is still being allocated there.
> 
> Thank you for this fantastic explanation and the bug fix!
> 
> Since IMHO it would be a shame to lose all these explanations, and 
> because it's been exposed to -next for weeks without getting reported, 
> I've applied your fix to tip:x86/fpu after changeloggifying it. I've 
> also added your SOB, if that's fine with you. Let's not destroy genuine 
> Git history, let's not lose credit and let's not lose all these details 
> by squashing this fix into the buggy commit...

Okay, sounds good. :) Yeah, please consider it:

Signed-off-by: Kees Cook <kees@kernel.org>

Thanks!

-- 
Kees Cook