This series fixes two issues in XSAVES offset calculation: - In setup_xstate_features(), supervisor xstate offsets are left as -1's, but still being tracked as last_good_offset; - In setup_xstate_comp(), alignments are being added to disabled xstate offsets. These issues have not been visible because supervisor xstates have not been enabled and there is no xfeature requiring alignment. They are triggered only when adding an aligned non-supervisor xstate after CET [1] supervisor states. To detect future potential issues, also add a patch to issue warnings when checking alignments of disabled xfeatures. Details are in each patch's commit log. [1] CET patches: https://lkml.kernel.org/r/20190813205225.12032-1-yu-cheng.yu@intel.com/ https://lkml.kernel.org/r/20190813205359.12196-1-yu-cheng.yu@intel.com/ Yu-cheng Yu (3): x86/fpu/xstate: Fix last_good_offset in setup_xstate_features() x86/fpu/xstate: Fix XSAVES offsets in setup_xstate_comp() x86/fpu/xstate: WARN_ONCE on checking alignment of disabled xfeatures arch/x86/kernel/fpu/xstate.c | 61 +++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 29 deletions(-) -- 2.21.0
The function setup_xstate_features() uses CPUID to find each xfeature's standard-format offset and size. Since XSAVES always uses the compacted format, supervisor xstates are *NEVER* in the standard-format and their offsets are left as -1's. However, they are still being tracked as last_good_offset. Fix it by tracking only user xstate offsets. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> --- arch/x86/kernel/fpu/xstate.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index a1806598aaa4..3ef3603bcfc5 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -265,22 +265,26 @@ static void __init setup_xstate_features(void) cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx); + xstate_sizes[i] = eax; + /* * If an xfeature is supervisor state, the offset * in EBX is invalid. We leave it to -1. */ - if (xfeature_is_user(i)) + if (xfeature_is_user(i)) { xstate_offsets[i] = ebx; - xstate_sizes[i] = eax; - /* - * In our xstate size checks, we assume that the - * highest-numbered xstate feature has the - * highest offset in the buffer. Ensure it does. - */ - WARN_ONCE(last_good_offset > xstate_offsets[i], - "x86/fpu: misordered xstate at %d\n", last_good_offset); - last_good_offset = xstate_offsets[i]; + /* + * In our xstate size checks, we assume that the + * highest-numbered xstate feature has the + * highest offset in the buffer. Ensure it does. + */ + WARN_ONCE(last_good_offset > xstate_offsets[i], + "x86/fpu: misordered xstate at %d\n", + last_good_offset); + + last_good_offset = xstate_offsets[i]; + } } } -- 2.21.0
In setup_xstate_comp(), each XSAVES component offset starts from the end of its preceding component plus alignment. A disabled feature does not take space and its offset should be set to the end of its preceding one with no alignment. However, in this case, alignment is incorrectly added to the offset, which can cause the next component to have a wrong offset. This problem has not been visible because currently there is no xfeature requiring alignment. Fix it by tracking the next starting offset only from enabled xfeatures. To make it clear, also change the function name to setup_xstate_comp_ offsets(). Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> --- arch/x86/kernel/fpu/xstate.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index 3ef3603bcfc5..73314e8fce02 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -344,9 +344,9 @@ static int xfeature_is_aligned(int xfeature_nr) * xsave area. This supports both standard format and compacted format * of the xsave aread. */ -static void __init setup_xstate_comp(void) +static void __init setup_xstate_comp_offsets(void) { - unsigned int xstate_comp_sizes[XFEATURE_MAX]; + unsigned int next_offset; int i; /* @@ -360,31 +360,23 @@ static void __init setup_xstate_comp(void) if (!boot_cpu_has(X86_FEATURE_XSAVES)) { for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) { - if (xfeature_enabled(i)) { + if (xfeature_enabled(i)) xstate_comp_offsets[i] = xstate_offsets[i]; - xstate_comp_sizes[i] = xstate_sizes[i]; - } } return; } - xstate_comp_offsets[FIRST_EXTENDED_XFEATURE] = - FXSAVE_SIZE + XSAVE_HDR_SIZE; + next_offset = FXSAVE_SIZE + XSAVE_HDR_SIZE; for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) { - if (xfeature_enabled(i)) - xstate_comp_sizes[i] = xstate_sizes[i]; - else - xstate_comp_sizes[i] = 0; + if (!xfeature_enabled(i)) + continue; - if (i > FIRST_EXTENDED_XFEATURE) { - xstate_comp_offsets[i] = xstate_comp_offsets[i-1] - + xstate_comp_sizes[i-1]; + if (xfeature_is_aligned(i)) + next_offset = ALIGN(next_offset, 64); - if (xfeature_is_aligned(i)) - xstate_comp_offsets[i] = - ALIGN(xstate_comp_offsets[i], 64); - } + xstate_comp_offsets[i] = next_offset; + next_offset += xstate_sizes[i]; } } @@ -778,7 +770,7 @@ void __init fpu__init_system_xstate(void) fpu__init_prepare_fx_sw_frame(); setup_init_fpu_buf(); - setup_xstate_comp(); + setup_xstate_comp_offsets(); print_xstate_offset_size(); pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n", -- 2.21.0
An XSAVES component's alignment/offset is meaningful only when the feature is enabled. Return zero and WARN_ONCE on checking alignment of disabled features. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> --- arch/x86/kernel/fpu/xstate.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index 73314e8fce02..4fa494073289 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -330,6 +330,13 @@ static int xfeature_is_aligned(int xfeature_nr) u32 eax, ebx, ecx, edx; CHECK_XFEATURE(xfeature_nr); + + if (!xfeature_enabled(xfeature_nr)) { + WARN_ONCE(1, "Checking alignment of disabled xfeature %d\n", + xfeature_nr); + return 0; + } + cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx); /* * The value returned by ECX[1] indicates the alignment -- 2.21.0
On Thu, Jan 09, 2020 at 01:14:50PM -0800, Yu-cheng Yu wrote: > The function setup_xstate_features() uses CPUID to find each xfeature's > standard-format offset and size. Since XSAVES always uses the compacted > format, supervisor xstates are *NEVER* in the standard-format and their > offsets are left as -1's. However, they are still being tracked as > last_good_offset. > > Fix it by tracking only user xstate offsets. > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> > --- > arch/x86/kernel/fpu/xstate.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) How about cleaning it up some more, while at it? --- From c12e13dcd814023a903399ec5ac2e7082d664b8b Mon Sep 17 00:00:00 2001 From: Yu-cheng Yu <yu-cheng.yu@intel.com> Date: Thu, 9 Jan 2020 13:14:50 -0800 Subject: [PATCH] x86/fpu/xstate: Fix last_good_offset in setup_xstate_features() The function setup_xstate_features() uses CPUID to find each xfeature's standard-format offset and size. Since XSAVES always uses the compacted format, supervisor xstates are *NEVER* in the standard-format and their offsets are left as -1's. However, they are still being tracked as last_good_offset. Fix it by tracking only user xstate offsets. [ bp: Use xfeature_is_supervisor() and save an indentation level. Drop now unused xfeature_is_user(). ] Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> Link: https://lkml.kernel.org/r/20200109211452.27369-2-yu-cheng.yu@intel.com --- arch/x86/kernel/fpu/xstate.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index a1806598aaa4..fe67cabfb4a1 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -120,11 +120,6 @@ static bool xfeature_is_supervisor(int xfeature_nr) return ecx & 1; } -static bool xfeature_is_user(int xfeature_nr) -{ - return !xfeature_is_supervisor(xfeature_nr); -} - /* * When executing XSAVEOPT (or other optimized XSAVE instructions), if * a processor implementation detects that an FPU state component is still @@ -265,21 +260,25 @@ static void __init setup_xstate_features(void) cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx); + xstate_sizes[i] = eax; + /* - * If an xfeature is supervisor state, the offset - * in EBX is invalid. We leave it to -1. + * If an xfeature is supervisor state, the offset in EBX is + * invalid, leave it to -1. */ - if (xfeature_is_user(i)) - xstate_offsets[i] = ebx; + if (xfeature_is_supervisor(i)) + continue; + + xstate_offsets[i] = ebx; - xstate_sizes[i] = eax; /* - * In our xstate size checks, we assume that the - * highest-numbered xstate feature has the - * highest offset in the buffer. Ensure it does. + * In our xstate size checks, we assume that the highest-numbered + * xstate feature has the highest offset in the buffer. Ensure + * it does. */ WARN_ONCE(last_good_offset > xstate_offsets[i], - "x86/fpu: misordered xstate at %d\n", last_good_offset); + "x86/fpu: misordered xstate at %d\n", last_good_offset); + last_good_offset = xstate_offsets[i]; } } -- 2.21.0 -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
The following commit has been merged into the x86/fpu branch of tip: Commit-ID: e70b100806d63fb79775858ea92e1a716da46186 Gitweb: https://git.kernel.org/tip/e70b100806d63fb79775858ea92e1a716da46186 Author: Yu-cheng Yu <yu-cheng.yu@intel.com> AuthorDate: Thu, 09 Jan 2020 13:14:52 -08:00 Committer: Borislav Petkov <bp@suse.de> CommitterDate: Wed, 12 Feb 2020 15:43:34 +01:00 x86/fpu/xstate: Warn when checking alignment of disabled xfeatures An XSAVES component's alignment/offset is meaningful only when the feature is enabled. Return zero and WARN_ONCE on checking alignment of disabled features. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> Link: https://lkml.kernel.org/r/20200109211452.27369-4-yu-cheng.yu@intel.com --- arch/x86/kernel/fpu/xstate.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index edcaacd..73fe597 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -325,6 +325,13 @@ static int xfeature_is_aligned(int xfeature_nr) u32 eax, ebx, ecx, edx; CHECK_XFEATURE(xfeature_nr); + + if (!xfeature_enabled(xfeature_nr)) { + WARN_ONCE(1, "Checking alignment of disabled xfeature %d\n", + xfeature_nr); + return 0; + } + cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx); /* * The value returned by ECX[1] indicates the alignment
The following commit has been merged into the x86/fpu branch of tip: Commit-ID: 49a91d61aed1db01097b51a24c77137eb348a0bf Gitweb: https://git.kernel.org/tip/49a91d61aed1db01097b51a24c77137eb348a0bf Author: Yu-cheng Yu <yu-cheng.yu@intel.com> AuthorDate: Thu, 09 Jan 2020 13:14:51 -08:00 Committer: Borislav Petkov <bp@suse.de> CommitterDate: Wed, 12 Feb 2020 15:43:31 +01:00 x86/fpu/xstate: Fix XSAVES offsets in setup_xstate_comp() In setup_xstate_comp(), each XSAVES component offset starts from the end of its preceding component plus alignment. A disabled feature does not take space and its offset should be set to the end of its preceding one with no alignment. However, in this case, alignment is incorrectly added to the offset, which can cause the next component to have a wrong offset. This problem has not been visible because currently there is no xfeature requiring alignment. Fix it by tracking the next starting offset only from enabled xfeatures. To make it clear, also change the function name to setup_xstate_comp_offsets(). [ bp: Fix a typo in the comment above it, while at it. ] Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> Link: https://lkml.kernel.org/r/20200109211452.27369-3-yu-cheng.yu@intel.com --- arch/x86/kernel/fpu/xstate.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index fe67cab..edcaacd 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -337,11 +337,11 @@ static int xfeature_is_aligned(int xfeature_nr) /* * This function sets up offsets and sizes of all extended states in * xsave area. This supports both standard format and compacted format - * of the xsave aread. + * of the xsave area. */ -static void __init setup_xstate_comp(void) +static void __init setup_xstate_comp_offsets(void) { - unsigned int xstate_comp_sizes[XFEATURE_MAX]; + unsigned int next_offset; int i; /* @@ -355,31 +355,23 @@ static void __init setup_xstate_comp(void) if (!boot_cpu_has(X86_FEATURE_XSAVES)) { for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) { - if (xfeature_enabled(i)) { + if (xfeature_enabled(i)) xstate_comp_offsets[i] = xstate_offsets[i]; - xstate_comp_sizes[i] = xstate_sizes[i]; - } } return; } - xstate_comp_offsets[FIRST_EXTENDED_XFEATURE] = - FXSAVE_SIZE + XSAVE_HDR_SIZE; + next_offset = FXSAVE_SIZE + XSAVE_HDR_SIZE; for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) { - if (xfeature_enabled(i)) - xstate_comp_sizes[i] = xstate_sizes[i]; - else - xstate_comp_sizes[i] = 0; + if (!xfeature_enabled(i)) + continue; - if (i > FIRST_EXTENDED_XFEATURE) { - xstate_comp_offsets[i] = xstate_comp_offsets[i-1] - + xstate_comp_sizes[i-1]; + if (xfeature_is_aligned(i)) + next_offset = ALIGN(next_offset, 64); - if (xfeature_is_aligned(i)) - xstate_comp_offsets[i] = - ALIGN(xstate_comp_offsets[i], 64); - } + xstate_comp_offsets[i] = next_offset; + next_offset += xstate_sizes[i]; } } @@ -773,7 +765,7 @@ void __init fpu__init_system_xstate(void) fpu__init_prepare_fx_sw_frame(); setup_init_fpu_buf(); - setup_xstate_comp(); + setup_xstate_comp_offsets(); print_xstate_offset_size(); pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n",
The following commit has been merged into the x86/fpu branch of tip: Commit-ID: c12e13dcd814023a903399ec5ac2e7082d664b8b Gitweb: https://git.kernel.org/tip/c12e13dcd814023a903399ec5ac2e7082d664b8b Author: Yu-cheng Yu <yu-cheng.yu@intel.com> AuthorDate: Thu, 09 Jan 2020 13:14:50 -08:00 Committer: Borislav Petkov <bp@suse.de> CommitterDate: Tue, 11 Feb 2020 19:54:04 +01:00 x86/fpu/xstate: Fix last_good_offset in setup_xstate_features() The function setup_xstate_features() uses CPUID to find each xfeature's standard-format offset and size. Since XSAVES always uses the compacted format, supervisor xstates are *NEVER* in the standard-format and their offsets are left as -1's. However, they are still being tracked as last_good_offset. Fix it by tracking only user xstate offsets. [ bp: Use xfeature_is_supervisor() and save an indentation level. Drop now unused xfeature_is_user(). ] Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com> Link: https://lkml.kernel.org/r/20200109211452.27369-2-yu-cheng.yu@intel.com --- arch/x86/kernel/fpu/xstate.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index a180659..fe67cab 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -120,11 +120,6 @@ static bool xfeature_is_supervisor(int xfeature_nr) return ecx & 1; } -static bool xfeature_is_user(int xfeature_nr) -{ - return !xfeature_is_supervisor(xfeature_nr); -} - /* * When executing XSAVEOPT (or other optimized XSAVE instructions), if * a processor implementation detects that an FPU state component is still @@ -265,21 +260,25 @@ static void __init setup_xstate_features(void) cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx); + xstate_sizes[i] = eax; + /* - * If an xfeature is supervisor state, the offset - * in EBX is invalid. We leave it to -1. + * If an xfeature is supervisor state, the offset in EBX is + * invalid, leave it to -1. */ - if (xfeature_is_user(i)) - xstate_offsets[i] = ebx; + if (xfeature_is_supervisor(i)) + continue; + + xstate_offsets[i] = ebx; - xstate_sizes[i] = eax; /* - * In our xstate size checks, we assume that the - * highest-numbered xstate feature has the - * highest offset in the buffer. Ensure it does. + * In our xstate size checks, we assume that the highest-numbered + * xstate feature has the highest offset in the buffer. Ensure + * it does. */ WARN_ONCE(last_good_offset > xstate_offsets[i], - "x86/fpu: misordered xstate at %d\n", last_good_offset); + "x86/fpu: misordered xstate at %d\n", last_good_offset); + last_good_offset = xstate_offsets[i]; } }