Re: commit 81106b7e0b13 can break asm_int80_emulation on x86_64

From: Bert Karwatzki
Date: Fri Aug 09 2024 - 19:05:42 EST


Am Freitag, dem 09.08.2024 um 11:17 -0700 schrieb Linus Torvalds:
> On Fri, 9 Aug 2024 at 07:53, Bert Karwatzki <spasswolf@xxxxxx> wrote:
> >
> > So the problem seems to be that the kmem_cache object *s has usersize 0. This
> > should be impossible in theory as kmem_cache_create_usercopy() should print
> > a warning in case of (!usersize && useroffset).
>
> Following along from your original report:
>
> usercopy: Kernel memory overwrite attempt detected to SLUB object
> 'task_struct' (offset 3072, size 160)!
>
> IOW, the user copy code is unhappy about copying data from user mode
> into the task_struct allocation.
>
> Anyway, on x86-64 with that commit we now just do
>
> arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
> {
> *offset = 0;
> *size = 0;
> }
>
> and that seems to be the bug.
>
> It's still allocated together with that 'struct task_struct', even if
> it's no longer inside the 'struct thread_struct'.
>
> Ingo? I think it needs to be something like
>
> *offset = sizeof(struct task_struct)
> - offsetof(struct task_struct, thread)
> + offsetof(struct fpu, __fpstate.regs);
> *size = fpu_kernel_cfg.default_size;
>
> and the commit message of
>
> 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.
>
> is clearly not true. No, it's not embedded in 'struct task_struct',
> but it *is* still allocated together with it in the same slab if I
> read the code correctly (ie this part
>
> static void __init fpu__init_task_struct_size(void)
> {
> int task_size = sizeof(struct task_struct);
>
> task_size += sizeof(struct fpu);
> ..
> arch_task_struct_size = task_size;
>
>
> is because it's still all one single slab allocation.
>
> Linus

Yes, this seems to be the problem. As the old code (from linux-5.16-rc1 to
linux-6.11-rc2) used this

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;
}

I tried this as a potential solution:

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index bd0621210f63..f1d713de6dba 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -516,8 +516,9 @@ extern struct fpu *x86_task_fpu(struct task_struct *task);
static inline void
arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
{
- *offset = 0;
- *size = 0;
+ *offset = sizeof(struct thread_struct)
+ + offsetof(struct fpu, __fpstate.regs);
+ *size = fpu_kernel_cfg.default_size;
}

static inline void

and it seems to solve the problem (debug output from my previous printk patch):

[ T57380] copy_uabi_to_xstate 1261: calling copy_from buffer with offset =
0x200, size = 0x40
[ T57380] copy_from_buffer: calling copy_from_user with to = ffffadf495127d58
from = 000000003ffef840, n = 0x40
[ T57380] copy_uabi_to_xstate 1275: calling copy_from buffer with offset =
0x18, size = 0x8
[ T57380] copy_from_buffer: calling copy_from_user with to = ffffadf495127d50
from = 000000003ffef658, n = 0x8
[ T57380] copy_uabi_to_xstate 1300: calling copy_from buffer 0 with offset =
0x0, size = 0xa0, dst = ffff90d285ec7880, kbuf = 0000000000000000, ubuf =
000000003ffef640
[ T57380] copy_from_buffer: calling copy_from_user with to = ffff90d285ec7880
from = 000000003ffef640, n = 0xa0

Here the bug would happen in linux-next-20240806 which seems to be avoided by
the patch.

[ T57380] copy_uabi_to_xstate 1300: calling copy_from buffer 1 with offset =
0xa0, size = 0x100, dst = ffff90d285ec7920, kbuf = 0000000000000000, ubuf =
000000003ffef640
[ T57380] copy_from_buffer: calling copy_from_user with to = ffff90d285ec7920
from = 000000003ffef6e0, n = 0x100

Bert Karwatzki