[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <19a94f88f1bc66bb81dbf5dd72083d03ca5090e9.camel@intel.com>
Date: Thu, 19 Dec 2019 08:44:08 -0800
From: Yu-cheng Yu <yu-cheng.yu@...el.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Tony Luck <tony.luck@...el.com>,
Andy Lutomirski <luto@...nel.org>,
Borislav Petkov <bp@...en8.de>,
Rik van Riel <riel@...riel.com>,
"Ravi V. Shankar" <ravi.v.shankar@...el.com>,
Fenghua Yu <fenghua.yu@...el.com>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when
__fpu_restore_sig() fails
On Thu, 2019-12-19 at 15:22 +0100, Sebastian Andrzej Siewior wrote:
> On 2019-12-18 12:53:59 [-0800], Yu-cheng Yu wrote:
> > I could have explained this better, sorry! I will explain the first
> > case below; other cases are similar.
> >
> > In copy_user_to_fpregs_zeroing(), we have:
> >
> > if (user_xsave()) {
> > ...
> > if (unlikely(init_bv))
> > copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
> > return copy_user_to_xregs(buf, xbv);
> > ...
> > }
> >
> > The copy_user_to_xregs() may fail, and when that happens, before going to
> > the slow path, there is fpregs_unlock() and context switches may happen.
>
> The context switch may only happen after fpregs_unlock().
>
> > However, at this point, fpu_fpregs_owner_ctx has not been changed; it could
> > still be another task's FPU.
>
> TIF_NEED_FPU_LOAD is set for the task in __fpu__restore_sig() and its
> context (__fpu_invalidate_fpregs_state()) has been invalidated. So the
> FPU register may contain another task's content and
> fpu_fpregs_owner_ctx points to another context.
>
> > For this to happen and to be detected, the user
> > stack page needs to be non-present, fpu_fpregs_owner_ctx need to be another task,
> > and that other task needs to be able to detect its registers are modified.
> > The last factor is not easy to reproduce, and a CET control-protection fault
> > helps.
>
> So far everything is legal. However. If there is a context switch before
> fpregs_lock() then this is bad before we don't account for that.
> So that:
>
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -352,6 +352,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> fpregs_unlock();
> return 0;
> }
> + fpregs_deactivate(fpu);
> fpregs_unlock();
> }
>
> @@ -403,6 +404,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> }
> if (!ret)
> fpregs_mark_activate();
> + else
> + fpregs_deactivate(fpu);
> fpregs_unlock();
>
> err_out:
>
>
> Should be enough.
Yes, this works. But then everywhere that calls copy_*_to_xregs_*() etc. needs to be checked.
Are there other alternatives?
Yu-cheng
Powered by blists - more mailing lists