linux-kernel - Re: [RFC PATCH] x86: Drop fpregs lock before inheriting FPU permissions during clone

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: <87o7tg8584.ffs@tglx>
Date:   Wed, 09 Nov 2022 17:25:47 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Mel Gorman <mgorman@...hsingularity.net>
Cc:     "Chang S. Bae" <chang.seok.bae@...el.com>,
        Borislav Petkov <bp@...e.de>, Mike Galbraith <efault@....de>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux-RT <linux-rt-users@...r.kernel.org>
Subject: Re: [RFC PATCH] x86: Drop fpregs lock before inheriting FPU
 permissions during clone

On Wed, Nov 09 2022 at 11:30, Mel Gorman wrote:
>    BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46
...
>   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 PREEMPT_RT disables
> preemption only so the spin_lock_irq() in fpu_inherit_perms is unsafe
> and converting siglock to raw spinlock would be an unwelcome change.
> This problem exists since commit 9e798e9aa14c ("x86/fpu: Prepare fpu_clone()
> for dynamically enabled features"). While the bug triggering is probably a
> mistake for the affected machine and due to a bug that is not in mainline,
> spin_lock_irq within a preempt_disable section on PREEMPT_RT is problematic.
>
> In this specific context, it may not be necessary to hold fpregs_lock at
> all. The lock is necessary when editing the FPU registers or a tasks fpstate
> but in this case, the only write of any FP state in fpu_inherit_perms is
> for the new child which is not running yet so it 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. The siglock still needs
> to be acquired by fpu_inherit_perms as the read of the parents permissions
> has to be serialised.

That's correct and siglock is the real protection for the permissions.

> This is not tested as I did not access to a machine with Intel's
> eXtended Feature Disable (XFD) feature that enables the relevant path
> in fpu_inherit_perms and the bug is against a non-mainline kernel.

It's still entirely correct on mainline as there is no requirement to
hold fpregs_lock in this case

> Reported-by: Mike Galbraith <efault@....de>
> Signed-off-by: Mel Gorman <mgorman@...hsingularity.net>

Reviewed-by: Thomas Gleixner <tglx@...utronix.de>