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

[Ingo Molnar]

* Kees Cook <kees@xxxxxxxxxx> 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...

Thanks,

	Ingo