On Tue, May 10, 2016 at 01:04:41PM +0200, Borislav Petkov wrote: > On Mon, May 09, 2016 at 01:45:58PM -0700, Yu-cheng Yu wrote: > > If "xsaves" is enabled, kernel always uses compacted format of xsave area. > > But user space still uses standard format of xsave area. Thus, xstate size > > in kernel's xsave area is smaller than xstate size in user's xsave area. > > The xstate in signal frame should be in standard format for user's signal > > handler to access. > > > > In no "xsaves" case, xsave area in both user space and kernel space are in > > standard format. Therefore, user's and kernel's xstate sizes are equal. > > > > In "xsaves" case, xsave area in user space is in standard format while > > xsave area in kernel space is in compacted format. Therefore, kernel's > > xstate size is smaller than user's xstate size. > > So this repeats what the first paragraph said. > > > So here is the problem: currently kernel assumes its own xstate size is > > signal frame's xstate size. This is not a problem in no "xsaves" case. > > ^ > the > > This whole text is missing a bunch of "the"s... > > > It is an issue in "xsaves" case because kernel's xstate size is smaller > ^ ^ > the the > > and so on... > > > > than user's xstate size. In fpu__alloc_mathframe(), a smaller fpstate > > buffer is allocated for the standard format xstate in signal frame. > > Then kernel saves only part of xstate registers into this smaller > > user's fpstate buffer and user will see part of the xstate registers in > > signal context. Similar issue happens after returning from signal handler: > > kernel will only restore part of xstate registers from user's fpstate > > buffer in signal frame. > > > > This patch defines and uses user_xstate_size for xstate size in signal > > frame. It's read from returned value in ebx from CPUID leaf 0x0D subleaf > > 0x0. This is maximum size required by enabled states in XCR0 and may be > > different from ecx when states at the end of the xsave area are not > > enabled. This value indicates the size required for XSAVE to save all > > supported user states in legacy/standard format. > > > > Signed-off-by: Fenghua Yu <[email protected]> > > Signed-off-by: Yu-cheng Yu <[email protected]> > > That SOB chain needs clarification: if Fenghua is the author, the patch > should contain his From: at the top. If it is based on an earlier patch > from him, commit message should say: > > Based on an earlier patch from Fenghua... without the SOB. > > > Reviewed-by: Dave Hansen <[email protected]> > > --- > > arch/x86/include/asm/fpu/xstate.h | 1 - > > arch/x86/include/asm/processor.h | 1 + > > arch/x86/kernel/fpu/init.c | 5 ++- > > arch/x86/kernel/fpu/signal.c | 26 ++++++++++---- > > arch/x86/kernel/fpu/xstate.c | 71 > > ++++++++++++++++++++++++--------------- > > 5 files changed, 67 insertions(+), 37 deletions(-) > > > > diff --git a/arch/x86/include/asm/fpu/xstate.h > > b/arch/x86/include/asm/fpu/xstate.h > > index 38951b0..16df2c4 100644 > > --- a/arch/x86/include/asm/fpu/xstate.h > > +++ b/arch/x86/include/asm/fpu/xstate.h > > @@ -39,7 +39,6 @@ > > #define REX_PREFIX > > #endif > > > > -extern unsigned int xstate_size; > > extern u64 xfeatures_mask; > > extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS]; > > > > diff --git a/arch/x86/include/asm/processor.h > > b/arch/x86/include/asm/processor.h > > index 9264476..132b4ca 100644 > > --- a/arch/x86/include/asm/processor.h > > +++ b/arch/x86/include/asm/processor.h > > @@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack); > > #endif /* X86_64 */ > > > > extern unsigned int xstate_size; > > +extern unsigned int user_xstate_size; > > If this is going to be exported, let's prefix it pls: > > fpu_user_xstate_size > > or > > xstate_user_state_size > > or somesuch. > > And let's add a comment over its definition what exactly it represents. > I.e., the aspect about the signal frame... > > ... > > > @@ -591,7 +593,15 @@ static bool is_supported_xstate_size(unsigned int > > test_xstate_size) > > static int init_xstate_size(void) > > { > > /* Recompute the context size for enabled features: */ > > - unsigned int possible_xstate_size = calculate_xstate_size(); > > + unsigned int possible_xstate_size; > > + unsigned int xsave_size; > > + > > + xsave_size = get_xsave_size(); > > + > > + if (cpu_has_xsaves) > > if (boot_cpu_has(X86_FEATURE_XSAVES)) > > > + possible_xstate_size = get_xsaves_size(); > > + else > > + possible_xstate_size = xsave_size; > > > > /* Ensure we have the space to store all enabled: */ > > if (!is_supported_xstate_size(possible_xstate_size)) > > -- > Regards/Gruss, > Boris. > > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB > 21284 (AG Nürnberg) > --
This is actually a patch of Fenghua's, but I re-based it. I will fix it. Yu-cheng
