On Mon, May 05, 2025 at 01:31:59PM +0200, Ingo Molnar wrote: > > * Kees Cook <[email protected]> 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 <[email protected]> Thanks! -- Kees Cook
