| Message ID | 20221203003606.6838-7-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Fri, Dec 02, 2022 at 04:35:33PM -0800, 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. > > 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. > > Tested-by: Pengfei Xu <pengfei.xu@intel.com> > Tested-by: John Allen <john.allen@amd.com> > 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 Fri, Dec 02, 2022 at 04:35:33PM -0800, Rick Edgecombe wrote: > +void fpregs_lock_and_load(void) Fun naming :) > +{ > + /* > + * fpregs_lock() only disables preemption (mostly). So modifing state Unknown word [modifing] in comment. Suggestions: ['modifying',... > + * 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(); So it locks them here... /me goes further into the patchset aha, and the counterpart of this function is fpregs_unlock() so everything gets sandwitched between the two. Ok, I guess. > +EXPORT_SYMBOL_GPL(fpregs_lock_and_load); Exported for KVM?
On Tue, 2022-12-20 at 13:04 +0100, Borislav Petkov wrote: > On Fri, Dec 02, 2022 at 04:35:33PM -0800, Rick Edgecombe wrote: > > +void fpregs_lock_and_load(void) > > Fun naming :) Yea. Suggested by Thomas. > > > +{ > > + /* > > + * fpregs_lock() only disables preemption (mostly). So > > modifing state > > Unknown word [modifing] in comment. > Suggestions: ['modifying',... Sorry about this and the others. I get spelling errors in checkpatch, so I must have dictionary issues or something. > > > + * 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(); > > So it locks them here... > > /me goes further into the patchset > aha, and the counterpart of this function is fpregs_unlock() so > everything gets sandwitched between the two. > > Ok, I guess. > > > +EXPORT_SYMBOL_GPL(fpregs_lock_and_load); > > Exported for KVM? > Yes, the KVM series needed it. Part of the reasoning here was to provide some helpers to avoid mistakes in modifying xstate, so the general idea was that it should be available. I suppose that series could add the export though?
On Wed, Dec 21, 2022 at 12:03:39AM +0000, Edgecombe, Rick P wrote: > Sorry about this and the others. I get spelling errors in checkpatch, > so I must have dictionary issues or something. No worries, happens to everyone. > Yes, the KVM series needed it. Part of the reasoning here was to > provide some helpers to avoid mistakes in modifying xstate, so the > general idea was that it should be available. I suppose that series > could add the export though? Yeah, you do have a point. Preemptive exposure of functionality is not really needed, so yeah, let's add it when actually really needed. Thx.
diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h index 503a577814b2..aadc6893dcaa 100644 --- a/arch/x86/include/asm/fpu/api.h +++ b/arch/x86/include/asm/fpu/api.h @@ -82,6 +82,15 @@ static inline void fpregs_unlock(void) preempt_enable(); } +/* + * FPU state gets lazily restored before returning to userspace. So when in the + * kernel, the valid FPU state may be kept in the buffer. This function will force + * restore all the fpu state to the registers early if needed, and lock them from + * being automatically saved/restored. Then FPU state can be modified safely in the + * registers, before unlocking with fpregs_unlock(). + */ +void fpregs_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 9baa89a8877d..9af78e9d92a0 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -753,6 +753,25 @@ void switch_fpu_return(void) } EXPORT_SYMBOL_GPL(switch_fpu_return); +void fpregs_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(fpregs_lock_and_load); + #ifdef CONFIG_X86_DEBUG_FPU /* * If current FPU state according to its tracking (loaded FPU context on this