| Message ID | 20220929222936.14584-7-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadowstacks for userspace | expand |
On Thu, Sep 29, 2022 at 03:29:03PM -0700, Rick Edgecombe wrote: > Just like user xfeatures, supervisor xfeatures can be active in the > registers or present in the task FPU buffer. If the registers are > active, the registers can be modified directly. If the registers are > not active, the modification must be performed on the task FPU buffer. > > When the state is not active, the kernel could perform modifications > directly to the buffer. But in order for it to do that, it needs > to know where in the buffer the specific state it wants to modify is > located. Doing this is not robust against optimizations that compact > the FPU buffer, as each access would require computing where in the > buffer it is. > > The easiest way to modify supervisor xfeature data is to force restore > the registers and write directly to the MSRs. Often times this is just fine > anyway as the registers need to be restored before returning to userspace. > Do this for now, leaving buffer writing optimizations for the future. Just for my own clarity, does this mean lock/load _needs_ to happen before MSR access, or is it just a convenient place to do it? From later patches it seems it's a requirement during MSR access, which might be a good idea to detail here. It answers the question "when is this function needed?" > > Add a new function fpregs_lock_and_load() that can simultaneously call > fpregs_lock() and do this restore. Also perform some extra sanity > checks in this function since this will be used in non-fpu focused code. Nit: this is called "fpu_lock_and_load" in the patch itself. > > Suggested-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> Reviewed-by: Kees Cook <keescook@chromium.org>
On Mon, 2022-10-03 at 10:48 -0700, Kees Cook wrote: > > The easiest way to modify supervisor xfeature data is to force > > restore > > the registers and write directly to the MSRs. Often times this is > > just fine > > anyway as the registers need to be restored before returning to > > userspace. > > Do this for now, leaving buffer writing optimizations for the > > future. > > Just for my own clarity, does this mean lock/load _needs_ to happen > before MSR access, or is it just a convenient place to do it? From > later > patches it seems it's a requirement during MSR access, which might be > a > good idea to detail here. It answers the question "when is this > function > needed?" The CET state is xsaves managed. It gets lazily restored before returning to userspace with the rest of the fpu stuff. This function will force restore all the fpu state to the registers early and lock them from being automatically saved/restored. Then the tasks CET state can be modified in the MSRs, before unlocking the fpregs. Last time I tried to modify the state directly in the xsave buffer when it was efficient, but it had issues and Thomas suggested this. > > > > > Add a new function fpregs_lock_and_load() that can simultaneously > > call > > fpregs_lock() and do this restore. Also perform some extra sanity > > checks in this function since this will be used in non-fpu focused > > code. > > Nit: this is called "fpu_lock_and_load" in the patch itself. Oops, thanks.
On Mon, Oct 03, 2022 at 08:05:13PM +0000, Edgecombe, Rick P wrote: > On Mon, 2022-10-03 at 10:48 -0700, Kees Cook wrote: > > > The easiest way to modify supervisor xfeature data is to force > > > restore > > > the registers and write directly to the MSRs. Often times this is > > > just fine > > > anyway as the registers need to be restored before returning to > > > userspace. > > > Do this for now, leaving buffer writing optimizations for the > > > future. > > > > Just for my own clarity, does this mean lock/load _needs_ to happen > > before MSR access, or is it just a convenient place to do it? From > > later > > patches it seems it's a requirement during MSR access, which might be > > a > > good idea to detail here. It answers the question "when is this > > function > > needed?" > > The CET state is xsaves managed. It gets lazily restored before > returning to userspace with the rest of the fpu stuff. This function > will force restore all the fpu state to the registers early and lock > them from being automatically saved/restored. Then the tasks CET state > can be modified in the MSRs, before unlocking the fpregs. Last time I > tried to modify the state directly in the xsave buffer when it was > efficient, but it had issues and Thomas suggested this. Okay, gotcha. Thanks!
On 10/3/22 13:05, Edgecombe, Rick P wrote: > The CET state is xsaves managed. It gets lazily restored before > returning to userspace with the rest of the fpu stuff. This function > will force restore all the fpu state to the registers early and lock > them from being automatically saved/restored. Then the tasks CET state > can be modified in the MSRs, before unlocking the fpregs. Last time I > tried to modify the state directly in the xsave buffer when it was > efficient, but it had issues and Thomas suggested this. Can you get the gist of this in a comment, please?
On Tue, 2022-10-04 at 07:18 -0700, Dave Hansen wrote: > On 10/3/22 13:05, Edgecombe, Rick P wrote: > > The CET state is xsaves managed. It gets lazily restored before > > returning to userspace with the rest of the fpu stuff. This > > function > > will force restore all the fpu state to the registers early and > > lock > > them from being automatically saved/restored. Then the tasks CET > > state > > can be modified in the MSRs, before unlocking the fpregs. Last time > > I > > tried to modify the state directly in the xsave buffer when it was > > efficient, but it had issues and Thomas suggested this. > > Can you get the gist of this in a comment, please? Sure.
diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h index 503a577814b2..3a86ee18ae99 100644 --- a/arch/x86/include/asm/fpu/api.h +++ b/arch/x86/include/asm/fpu/api.h @@ -82,6 +82,12 @@ static inline void fpregs_unlock(void) preempt_enable(); } +/* + * Lock and load the fpu state into the registers, if they are not already + * loaded. + */ +void fpu_lock_and_load(void); + #ifdef CONFIG_X86_DEBUG_FPU extern void fpregs_assert_state_consistent(void); #else diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index 3b28c5b25e12..778d3054ccc7 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -756,6 +756,25 @@ void switch_fpu_return(void) } EXPORT_SYMBOL_GPL(switch_fpu_return); +void fpu_lock_and_load(void) +{ + /* + * fpregs_lock() only disables preemption (mostly). So modifing state + * in an interrupt could screw up some in progress fpregs operation, + * but appear to work. Warn about it. + */ + WARN_ON_ONCE(!irq_fpu_usable()); + WARN_ON_ONCE(current->flags & PF_KTHREAD); + + fpregs_lock(); + + fpregs_assert_state_consistent(); + + if (test_thread_flag(TIF_NEED_FPU_LOAD)) + fpregs_restore_userregs(); +} +EXPORT_SYMBOL_GPL(fpu_lock_and_load); + #ifdef CONFIG_X86_DEBUG_FPU /* * If current FPU state according to its tracking (loaded FPU context on this
Just like user xfeatures, supervisor xfeatures can be active in the registers or present in the task FPU buffer. If the registers are active, the registers can be modified directly. If the registers are not active, the modification must be performed on the task FPU buffer. When the state is not active, the kernel could perform modifications directly to the buffer. But in order for it to do that, it needs to know where in the buffer the specific state it wants to modify is located. Doing this is not robust against optimizations that compact the FPU buffer, as each access would require computing where in the buffer it is. The easiest way to modify supervisor xfeature data is to force restore the registers and write directly to the MSRs. Often times this is just fine anyway as the registers need to be restored before returning to userspace. Do this for now, leaving buffer writing optimizations for the future. Add a new function fpregs_lock_and_load() that can simultaneously call fpregs_lock() and do this restore. Also perform some extra sanity checks in this function since this will be used in non-fpu focused code. Suggested-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v2: - Drop optimization of writing directly the buffer, and change API accordingly. - fpregs_lock_and_load() suggested by tglx - Some commit log verbiage from dhansen v1: - New patch. arch/x86/include/asm/fpu/api.h | 6 ++++++ arch/x86/kernel/fpu/core.c | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+)