linux-kernel - Re: commit 81106b7e0b13 can break asm_int80_emulation on x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8559e4f66f1e6f1d7e938cc7a833528f232872d2.camel@web.de>
Date: Sat, 10 Aug 2024 01:04:10 +0200
From: Bert Karwatzki <spasswolf@....de>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: mingo@...nel.org, akpm@...ux-foundation.org, Oleg Nesterov
 <oleg@...hat.com>,  Andy Lutomirski	 <luto@...nel.org>, Borislav Petkov
 <bp@...en8.de>, Fenghua Yu	 <fenghua.yu@...el.com>, "H . Peter Anvin"
 <hpa@...or.com>, Dave Hansen	 <dave.hansen@...ux.intel.com>, Thomas
 Gleixner <tglx@...utronix.de>, Uros Bizjak <ubizjak@...il.com>,
 linux-kernel@...r.kernel.org, peterz@...radead.org, spasswolf@....de
Subject: Re: commit 81106b7e0b13 can break asm_int80_emulation on x86_64

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