Re: commit 81106b7e0b13 can break asm_int80_emulation on x86_64

[Bert Karwatzki <spasswolf@web.de>]

Am Samstag, dem 10.08.2024 um 01:04 +0200 schrieb Bert Karwatzki:
> Am Freitag, dem 09.08.2024 um 11:17 -0700 schrieb Linus Torvalds:
> > On Fri, 9 Aug 2024 at 07:53, Bert Karwatzki <spasswolf@web.de> 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
>

After taking a closer look at the code I realized that my solution only works if
task_struct and thread_struct are not randomized while your solution

        *offset = sizeof(struct task_struct)
                - offsetof(struct task_struct, thread)
                + offsetof(struct fpu, __fpstate.regs);
        *size = fpu_kernel_cfg.default_size;

works in those cases, too. Here's a way to introdce this fix using
fpu_thread_struct_whitelist in arch/x86/kernel/fpu/init.c where we can use
sizeof(task_struct) without including additional headers:

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index bd0621210f63..87472bdce053 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -510,14 +510,12 @@ extern struct fpu *x86_task_fpu(struct task_struct *task);
 # define x86_task_fpu(task)    ((struct fpu *)((void *)(task) +
sizeof(*(task))))
 #endif

-/*
- * X86 doesn't need any embedded-FPU-struct quirks:
- */
+extern void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long
*size);
+
 static inline void
 arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
 {
-       *offset = 0;
-       *size = 0;
+       fpu_thread_struct_whitelist(offset, size);
 }

 static inline void
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 16b6611634c3..173670891f69 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -179,6 +179,18 @@ static void __init fpu__init_task_struct_size(void)
        arch_task_struct_size = task_size;
 }

+/*
+ * Whitelist the FPU register state embedded into task_struct for hardened
+ * usercopy.
+ */
+void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
+{
+       *offset = sizeof(struct task_struct)
+               - offsetof(struct task_struct, thread)
+               + offsetof(struct fpu, __fpstate.regs);
+       *size = fpu_kernel_cfg.default_size;
+}
+
 /*
  * Set up the user and kernel xstate sizes based on the legacy FPU context
size.
  *


Bert Karwatzki