linux-kernel - [PATCH] x86: Drop fpregs lock before inheriting FPU permissions

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-next>] [day] [month] [year] [list]

Message-ID: <20221110124400.zgymc2lnwqjukgfh@techsingularity.net>
Date:   Thu, 10 Nov 2022 12:44:00 +0000
From:   Mel Gorman <mgorman@...hsingularity.net>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     "Chang S. Bae" <chang.seok.bae@...el.com>,
        Borislav Petkov <bp@...e.de>, Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Mike Galbraith <efault@....de>,
        LKML <linux-kernel@...r.kernel.org>, Linux-X86 <x86@...nel.org>,
        Linux-RT <linux-rt-users@...r.kernel.org>
Subject: [PATCH] x86: Drop fpregs lock before inheriting FPU permissions

Mike Galbraith reported the following against an old fork of preempt-rt
but the same issue also applies to the current preempt-rt tree.

   BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46
   in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: systemd
   preempt_count: 1, expected: 0
   RCU nest depth: 0, expected: 0
   Preemption disabled at:
   fpu_clone+0xfa/0x480
   CPU: 6 PID: 1 Comm: systemd Tainted: G            E       (unreleased)
   Call Trace:
    <TASK>
    dump_stack_lvl+0x45/0x5b
    ? fpu_clone+0xfa/0x480
    __might_resched+0x165/0x200
    rt_spin_lock+0x2d/0x70
    fpu_clone+0x32a/0x480
    ? copy_thread+0xef/0x270
    ? copy_process+0xd2c/0x1c00
    ? shmem_alloc_inode+0x16/0x30
    ? kmem_cache_alloc+0x120/0x2a0
    ? kernel_clone+0x9b/0x460
    ? __do_sys_clone+0x72/0xa0
    ? do_syscall_64+0x58/0x80
    ? __x64_sys_rt_sigprocmask+0x93/0xd0
    ? syscall_exit_to_user_mode+0x18/0x40
    ? do_syscall_64+0x67/0x80
    ? syscall_exit_to_user_mode+0x18/0x40
    ? do_syscall_64+0x67/0x80
    ? syscall_exit_to_user_mode+0x18/0x40
    ? do_syscall_64+0x67/0x80
    ? exc_page_fault+0x6a/0x190
    ? entry_SYSCALL_64_after_hwframe+0x61/0xcb
    </TASK>

  The splat comes from fpu_inherit_perms() being called under fpregs_lock(),
  and us reaching the spin_lock_irq() therein due to fpu_state_size_dynamic()
  returning true despite static key __fpu_state_size_dynamic having never
  been enabled.

Mike's assessment looks correct. fpregs_lock on a PREEMPT_RT kernel disables
preemption so calling spin_lock_irq() in fpu_inherit_perms is unsafe. This
problem exists since commit 9e798e9aa14c ("x86/fpu: Prepare fpu_clone()
for dynamically enabled features"). Even though the original bug report
should not have enabled the paths at all, the bug still exists.

fpregs_lock is necessary when editing the FPU registers or a task's
FP state but it is not necessary for fpu_inherit_perms. The only write
of any FP state in fpu_inherit_perms is for the new child which is not
running yet and cannot context switch or be borrowed by a kernel thread
yet. Hence, fpregs_lock is not protecting anything in the new child until
clone() completes and can be dropped earlier. The siglock still needs to
be acquired by fpu_inherit_perms as the read of the parents permissions
has to be serialised.

Reported-by: Mike Galbraith <efault@....de>
Signed-off-by: Mel Gorman <mgorman@...hsingularity.net>
Reviewed-by: Thomas Gleixner <tglx@...utronix.de>
---
 arch/x86/kernel/fpu/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 3b28c5b25e12..d00db56a8868 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -605,9 +605,9 @@ int fpu_clone(struct task_struct *dst, unsigned long clone_flags, bool minimal)
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
 		fpregs_restore_userregs();
 	save_fpregs_to_fpstate(dst_fpu);
+	fpregs_unlock();
 	if (!(clone_flags & CLONE_THREAD))
 		fpu_inherit_perms(dst_fpu);
-	fpregs_unlock();

 	/*
 	 * Children never inherit PASID state.