IBRS patch series
Subsystems (4)
v2 cover IBRS patch series
Thanks to everyone for the feedback on the initial posting. This is an updated patchset and I hope I've captured all the review comments. I've done a lot of code clean up per everyone's comments. Please let me know if I've missed something.
The retpoline related changes is moved to the end of the patch series, so they can be taken out or changed easily without affecting the other patches.
Many people hate the multi-bits spec_ctrl_ibrs variable so I got rid of that and replace it with a dynamic_ibrs flag to indicate if we need to switch IBRS enter/exiting kernel which is more intuitive and also makes the code cleaner.
Peter/Andrea suggested that we use a static key to control the run time IBRS enabling/disabling with "STATIC_JUMP_IF_TRUE" kind
of construct. However, I had some concerns thatJUMP_LABEL config may be disabled and the construct cannot be used. I also encountered some
OOPs when I'm changing ibrs control state probably
related to changing the jump label branching. I haven't
had time to debug that so I left it out for now.
I will welcome some help here on a patch to get the static key thing working right.
v2.
1. Added missing feature enumeration in tools/arch/x86/include/asm/cpufeatures.h
2. Kernel entry macros label cleanup and move them to calling.h
3. Remove unnecessary irqs_diabled check in the mwait
4. Don't use a bit field base sys control variable to make ibrs enabling simpler and easier to understand
5. Corrected compile issues for firmware update code
6. Leave IBPB feature bits out from this patch series and will be added in its own set of patches later
Tim
---patch series details---
This patch series enables the basic detection and usage of x86 indirect branch speculation feature. It enables the indirect branch restricted speculation (IBRS) on kernel entry and disables it on exit. It enumerates the indirect branch prediction barrier (IBPB).
The x86 IBRS feature requires corresponding microcode support. It mitigates the variant 2 vulnerability described in
https://googleprojectzero.blogspot.com/2018/01/reading-privileged-memory-with-side.html
If IBRS is set, near returns and near indirect jumps/calls will not allow their predicted target address to be controlled by code that executed in a less privileged prediction mode before the IBRS mode was last written with a value of 1 or on another logical processor so long as all RSB entries from the previous less privileged prediction mode are overwritten.
Both retpoline and IBRS provides mitigation against variant 2 attacks, with IBRS being the most secured method but could incur more performance overhead compared to retpoline[1]. If you are very paranoid or you run on a CPU where IBRS=1 is cheaper, you may also want to run in "IBRS always" mode.
More detailed description of IBRS is described in the first patch.
It is applied on top of the page table isolation changes.
A run time and boot time control of the IBRS feature is provided
There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usage
Otherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
2. At run time
echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel (IBRS always)
[1] https://lkml.org/lkml/2018/1/4/174
Tim Chen (8):
x86/feature: Detect the x86 IBRS feature to control Speculation x86/enter: MACROS to set/clear IBRS
x86/enter: Use IBRS on syscall and interrupts x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature x86/idle: Disable IBRS entering idle and enable it on wakeup x86/microcode: Recheck IBRS features on microcode reload
x86: Do not use dynamic IBRS if retpoline is enabled
x86: Use IBRS for firmware update path
arch/x86/entry/calling.h | 104 +++++++++++++++ arch/x86/entry/entry_64.S | 23 ++++ arch/x86/entry/entry_64_compat.S | 8 ++ arch/x86/include/asm/apm.h | 6 + arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/efi.h | 17 ++- arch/x86/include/asm/msr-index.h | 4 + arch/x86/include/asm/mwait.h | 13 ++ arch/x86/include/asm/spec_ctrl.h | 54 ++++++++ arch/x86/kernel/cpu/Makefile | 1 + arch/x86/kernel/cpu/microcode/core.c | 4 + arch/x86/kernel/cpu/scattered.c | 3 + arch/x86/kernel/cpu/spec_ctrl.c | 209 +++++++++++++++++++++++++++++++ arch/x86/kernel/process.c | 9 +- tools/arch/x86/include/asm/cpufeatures.h | 1 + 15 files changed, 453 insertions(+), 4 deletions(-) create mode 100644 arch/x86/include/asm/spec_ctrl.h create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c
Signature · 1 line
2.9.4
PATCH v2 1/8 x86/feature: Detect the x86 IBRS feature to control Speculation
cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature IA32_SPEC_CTRL (0x48)IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS)
If IBRS is set, near returns and near indirect jumps/calls will not allow their predicted target address to be controlled by code that executed in a less privileged prediction mode before the IBRS mode was last written with a value of 1 or on another logical processor so long as all RSB entries from the previous less privileged prediction mode are overwritten.
* Thus a near indirect jump/call/return may be affected by code in a less privileged prediction mode that executed AFTER IBRS mode was last written with a value of 1
* There is no need to clear IBRS before writing it with a value of
1. Unconditionally writing it with a value of 1 after the prediction mode change is sufficient
* Note: IBRS is not required in order to isolate branch predictions for SMM or SGX enclaves
* Code executed by a sibling logical processor cannot control indirect jump/call/return predicted target when IBRS is set
* SMEP will prevent supervisor mode using RSB entries filled by user code; this can reduce the need for software to overwrite RSB entries
CPU performance could be reduced when running with IBRS set.
--- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++++ arch/x86/kernel/cpu/scattered.c | 1 + tools/arch/x86/include/asm/cpufeatures.h | 1 + 4 files changed, 7 insertions(+)
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -209,6 +209,7 @@#define X86_FEATURE_AVX512_4FMAPS ( 7*32+17)/* AVX-512 Multiply Accumulation Single precision */ #define X86_FEATURE_MBA ( 7*32+18)/* Memory Bandwidth Allocation */ +#define X86_FEATURE_SPEC_CTRL ( 7*32+19)/* Control Speculation Control */ /* Virtualization flags: Linux defined, word 8 */ #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0)/* Intel TPR Shadow */
arch/x86/include/asm/msr-index.h +4 -0
@@ -42,6 +42,10 @@#define MSR_PPIN_CTL 0x0000004e#define MSR_PPIN 0x0000004f +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0) +#define MSR_IA32_PERFCTR0 0x000000c1#define MSR_IA32_PERFCTR1 0x000000c2#define MSR_FSB_FREQ 0x000000cd
arch/x86/kernel/cpu/scattered.c +1 -0
@@ -24,6 +24,7 @@ static const struct cpuid_bit cpuid_bits[] = { { X86_FEATURE_INTEL_PT, CPUID_EBX, 25, 0x00000007, 0 }, { X86_FEATURE_AVX512_4VNNIW, CPUID_EDX, 2, 0x00000007, 0 }, { X86_FEATURE_AVX512_4FMAPS, CPUID_EDX, 3, 0x00000007, 0 }, + { X86_FEATURE_SPEC_CTRL, CPUID_EDX, 26, 0x00000007, 0 }, { X86_FEATURE_CAT_L3, CPUID_EBX, 1, 0x00000010, 0 }, { X86_FEATURE_CAT_L2, CPUID_EBX, 2, 0x00000010, 0 }, { X86_FEATURE_CDP_L3, CPUID_ECX, 2, 0x00000010, 1 },
tools/arch/x86/include/asm/cpufeatures.h +1 -0
@@ -208,6 +208,7 @@#define X86_FEATURE_AVX512_4FMAPS ( 7*32+17)/* AVX-512 Multiply Accumulation Single precision */ #define X86_FEATURE_MBA ( 7*32+18)/* Memory Bandwidth Allocation */ +#define X86_FEATURE_SPEC_CTRL ( 7*32+19)/* Control Speculation Control */ /* Virtualization flags: Linux defined, word 8 */ #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0)/* Intel TPR Shadow */
Signature · 1 line
2.9.4
Borislav Petkov
maintainerRe: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
On Fri, Jan 05, 2018 at 06:12:16PM -0800, Tim Chen wrote:
<--- This needs an introductory sentence here.
Quoted 1 line
cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature
You can write that as CPUID(7).RDX[26].
Quoted 2 lines
IA32_SPEC_CTRL(0x48)IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS)
Ah, those are MSRs. Please say so.
Quoted 4 lines
If IBRS is set, near returns and near indirect jumps/calls will not allow their predicted target address to be controlled by code that executed in a less privileged prediction mode before the IBRS mode was last written with a value of 1 or on another logical processor so long as all RSB
"RSB" is?
Quoted 5 lines
entries from the previous less privileged prediction mode are overwritten.
* Thus a near indirect jump/call/return may be affected by code in a less privileged prediction mode that executed AFTER IBRS mode was last written with a value of 1
End sentences with a full stop.
Quoted 3 lines
* There is no need to clear IBRS before writing it with a value of
1. Unconditionally writing it with a value of 1 after the prediction mode change is sufficient
This sounds strange. I know of funky MSRs like that but if it is not the case here, no need to mention it then.
Quoted 36 lines
* Note: IBRS is not required in order to isolate branch predictions for SMM or SGX enclaves
* Code executed by a sibling logical processor cannot control indirect jump/call/return predicted target when IBRS is set
* SMEP will prevent supervisor mode using RSB entries filled by user code; this can reduce the need for software to overwrite RSB entries
CPU performance could be reduced when running with IBRS set.
Signed-off-by Tim Chen <[email protected]>--- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++++ arch/x86/kernel/cpu/scattered.c | 1 + tools/arch/x86/include/asm/cpufeatures.h | 1 + 4 files changed, 7 insertions(+)arch/x86/include/asm/cpufeatures.h +1
-0@@ -209,6 +209,7 @@#define X86_FEATURE_AVX512_4FMAPS ( 7*32+17)/* AVX-512 Multiply Accumulation Single precision */ #define X86_FEATURE_MBA ( 7*32+18)/* Memory Bandwidth Allocation */ +#define X86_FEATURE_SPEC_CTRL ( 7*32+19)/* Control Speculation Control */ /* Virtualization flags: Linux defined, word 8 */ #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0)/* Intel TPR Shadow */ arch/x86/include/asm/msr-index.h +0
-0
arch/x86/include/asm/msr-index.h line 47
Quoted 6 lines
arch/x86/include/asm/msr-index.h +3
-0@@ -42,6 +42,10 @@#define MSR_PPIN_CTL 0x0000004e#define MSR_PPIN 0x0000004f +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0)
s/_FEATURE//
SPEC_CTRL_{ENABLE,DISABLE}_IBRS is good enough.
Quoted 4 lines
arch/x86/include/asm/msr-index.h +1
-0+#define MSR_IA32_PERFCTR0 0x000000c1#define MSR_IA32_PERFCTR1 0x000000c2#define MSR_FSB_FREQ 0x000000cd
...
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
Quoted 11 lines · Borislav Petkov
On Fri, Jan 05, 2018 at 06:12:16PM -0800, Tim Chen wrote:
<--- This needs an introductory sentence here.
Quoted 1 line
cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature
You can write that as CPUID(7).RDX[26].
Quoted 2 lines
IA32_SPEC_CTRL(0x48)IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS)
Quoted 8 lines
Ah, those are MSRs. Please say so.
Quoted 4 lines
If IBRS is set, near returns and near indirect jumps/calls will not allow their predicted target address to be controlled by code that executed in a less privileged prediction mode before the IBRS mode was last written with a value of 1 or on another logical processor so long as all RSB
"RSB" is?
RSB is return stack buffer, basically speculation addresses for return statement.
Quoted 61 lines
Quoted 6 lines
entries from the previous less privileged prediction mode are overwritten.
* Thus a near indirect jump/call/return may be affected by code in a less privileged prediction mode that executed AFTER IBRS mode was last written with a value of 1
End sentences with a full stop.
Quoted 3 lines
* There is no need to clear IBRS before writing it with a value of
1. Unconditionally writing it with a value of 1 after the prediction mode change is sufficientThis sounds strange. I know of funky MSRs like that but if it is not the case here, no need to mention it then.
Quoted 36 lines
* Note: IBRS is not required in order to isolate branch predictions for SMM or SGX enclaves
* Code executed by a sibling logical processor cannot control indirect jump/call/return predicted target when IBRS is set
* SMEP will prevent supervisor mode using RSB entries filled by user code; this can reduce the need for software to overwrite RSB entries
CPU performance could be reduced when running with IBRS set.
Signed-off-by Tim Chen <[email protected]>--- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++++ arch/x86/kernel/cpu/scattered.c | 1 + tools/arch/x86/include/asm/cpufeatures.h | 1 + 4 files changed, 7 insertions(+)arch/x86/include/asm/cpufeatures.h +1
-0@@ -209,6 +209,7 @@#define X86_FEATURE_AVX512_4FMAPS ( 7*32+17)/* AVX-512 Multiply Accumulation Single precision */ #define X86_FEATURE_MBA ( 7*32+18)/* Memory Bandwidth Allocation */ +#define X86_FEATURE_SPEC_CTRL ( 7*32+19)/* Control Speculation Control */ /* Virtualization flags: Linux defined, word 8 */ #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0)/* Intel TPR Shadow */ arch/x86/include/asm/msr-index.h +0
-0arch/x86/include/asm/msr-index.h line 47
Quoted 6 lines
arch/x86/include/asm/msr-index.h +3
-0@@ -42,6 +42,10 @@#define MSR_PPIN_CTL 0x0000004e#define MSR_PPIN 0x0000004f +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0)s/_FEATURE//
Quoted 8 lines
SPEC_CTRL_{ENABLE,DISABLE}_IBRS is good enough.
Quoted 4 lines
arch/x86/include/asm/msr-index.h +1
-0+#define MSR_IA32_PERFCTR0 0x000000c1#define MSR_IA32_PERFCTR1 0x000000c2#define MSR_FSB_FREQ 0x000000cd...
Thanks. will update the phrasing.
Tim
Borislav Petkov
maintainerRe: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
Quoted 2 lines · Tim Chen
RSB is return stack buffer, basically speculation addresses for return statement.
I had a very good idea what it was but maybe other readers might not. So please write out abbreviations on their first use.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
In reply to Borislav PetkovQuoted 6 lines · Borislav Petkov
arch/x86/include/asm/msr-index.h line 47
Quoted 3 lines
arch/x86/include/asm/msr-index.h +3
-0+#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0)s/_FEATURE//
SPEC_CTRL_{ENABLE,DISABLE}_IBRS is good enough.
FWIW, I like the idea of including "SPEC_CTRL_" because it matches the constant to the MSR name. But I don't care that much.
Borislav Petkov
maintainerRe: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
Quoted 2 lines · Dave Hansen
FWIW, I like the idea of including "
SPEC_CTRL_" because it matches the constant to the MSR name. But I don't care that much.
Sure - just the "FEATURE_" part of the string is redundant.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
In reply to Tim ChenQuoted 24 lines · Tim Chen
cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature
IA32_SPEC_CTRL(0x48)IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS)If IBRS is set, near returns and near indirect jumps/calls will not allow their predicted target address to be controlled by code that executed in a less privileged prediction mode before the IBRS mode was last written with a value of 1 or on another logical processor so long as all RSB entries from the previous less privileged prediction mode are overwritten.
* Thus a near indirect jump/call/return may be affected by code in a less privileged prediction mode that executed AFTER IBRS mode was last written with a value of 1
* There is no need to clear IBRS before writing it with a value of
1. Unconditionally writing it with a value of 1 after the prediction mode change is sufficient* Note: IBRS is not required in order to isolate branch predictions for SMM or SGX enclaves
* Code executed by a sibling logical processor cannot control indirect jump/call/return predicted target when IBRS is set
Quoted 6 lines
* SMEP will prevent supervisor mode using RSB entries filled by user code; this can reduce the need for software to overwrite RSB entries
CPU performance could be reduced when running with IBRS set.
Signed-off-by Tim Chen <[email protected]>
KVM also needs STIBP, can you please add it, or perhaps split cpuid(7,0).edx out to its own feature word?
Thanks,
Paolo
Quoted 59 lines
--- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++++ arch/x86/kernel/cpu/scattered.c | 1 + tools/arch/x86/include/asm/cpufeatures.h | 1 + 4 files changed, 7 insertions(+)arch/x86/include/asm/cpufeatures.h +1
-0@@ -209,6 +209,7 @@#define X86_FEATURE_AVX512_4FMAPS ( 7*32+17)/* AVX-512 Multiply Accumulation Single precision */ #define X86_FEATURE_MBA ( 7*32+18)/* Memory Bandwidth Allocation */ +#define X86_FEATURE_SPEC_CTRL ( 7*32+19)/* Control Speculation Control */ /* Virtualization flags: Linux defined, word 8 */ #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0)/* Intel TPR Shadow */ arch/x86/include/asm/msr-index.h +4
-0@@ -42,6 +42,10 @@#define MSR_PPIN_CTL 0x0000004e#define MSR_PPIN 0x0000004f +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0) +#define MSR_IA32_PERFCTR0 0x000000c1#define MSR_IA32_PERFCTR1 0x000000c2#define MSR_FSB_FREQ 0x000000cdarch/x86/kernel/cpu/scattered.c +1
-0@@ -24,6 +24,7 @@ static const struct cpuid_bit cpuid_bits[] = { { X86_FEATURE_INTEL_PT, CPUID_EBX, 25, 0x00000007, 0 }, { X86_FEATURE_AVX512_4VNNIW, CPUID_EDX, 2, 0x00000007, 0 }, { X86_FEATURE_AVX512_4FMAPS, CPUID_EDX, 3, 0x00000007, 0 }, + { X86_FEATURE_SPEC_CTRL, CPUID_EDX, 26, 0x00000007, 0 }, { X86_FEATURE_CAT_L3, CPUID_EBX, 1, 0x00000010, 0 }, { X86_FEATURE_CAT_L2, CPUID_EBX, 2, 0x00000010, 0 }, { X86_FEATURE_CDP_L3, CPUID_ECX, 2, 0x00000010, 1 },tools/arch/x86/include/asm/cpufeatures.h +1
-0@@ -208,6 +208,7 @@#define X86_FEATURE_AVX512_4FMAPS ( 7*32+17)/* AVX-512 Multiply Accumulation Single precision */ #define X86_FEATURE_MBA ( 7*32+18)/* Memory Bandwidth Allocation */ +#define X86_FEATURE_SPEC_CTRL ( 7*32+19)/* Control Speculation Control */ /* Virtualization flags: Linux defined, word 8 */ #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0)/* Intel TPR Shadow */
Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
In reply to Tim ChenQuoted 5 lines · Tim Chen
arch/x86/include/asm/msr-index.h +0
-0
arch/x86/include/asm/msr-index.h line 51
Quoted 10 lines
arch/x86/include/asm/msr-index.h +4
-0@@ -42,6 +42,10 @@#define MSR_PPIN_CTL 0x0000004e#define MSR_PPIN 0x0000004f +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0) +#define MSR_IA32_PERFCTR0 0x000000c1#define MSR_IA32_PERFCTR1 0x000000c2#define MSR_FSB_FREQ 0x000000cd
This is the patch that I have in my KVM series:
arch/x86/include/asm/msr-index.h +8 -1
@@ -464,8 +464,15 @@#define MSR_SMI_COUNT 0x00000034#define MSR_IA32_FEATURE_CONTROL 0x0000003a#define MSR_IA32_TSC_ADJUST 0x0000003b + +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0) + +#define MSR_IA32_PRED_CMD 0x00000049 +#define PRED_CMD_IBPB (1UL << 0) +#define MSR_IA32_BNDCFGS 0x00000d90 -#define MSR_IA32_BNDCFGS_RSVD 0x00000ffc#define MSR_IA32_XSS 0x00000da0
Tim, can you include it like this to avoid conflicts?
Thanks,
Paolo
Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
arch/x86/include/asm/msr-index.h line 470
Quoted 4 lines · Paolo Bonzini
arch/x86/include/asm/msr-index.h +4
-0+ +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0)
Boris requested that the name for ENABLE/DISABLE to be shortened to SPEC_CTRL_{ENABLE,DISABLE}_IBRS. I'm planning to do that for the next patchset update.
arch/x86/include/asm/msr-index.h line 474
Quoted 4 lines
arch/x86/include/asm/msr-index.h +4
-0+ +#define MSR_IA32_PRED_CMD 0x00000049 +#define PRED_CMD_IBPB (1UL << 0) +
The IBPB stuff should be enumerated separately. Feel free to create a separate patch for this chunk.
Thanks.
Tim
Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
Quoted 11 lines · Tim Chen
arch/x86/include/asm/msr-index.h line 470
Quoted 4 lines · Paolo Bonzini
arch/x86/include/asm/msr-index.h +4
-0+ +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0)Boris requested that the name for ENABLE/DISABLE to be shortened to SPEC_CTRL_{ENABLE,DISABLE}_IBRS. I'm planning to do that for the next patchset update.
Were you also planning to include KVM patches in your patchset sooner or later? Or would that be a separate submission?
(Either is fine; in the latter case, I assume I'll get a topic branch from the tip tree).
Quoted 7 lines
arch/x86/include/asm/msr-index.h line 474
Quoted 4 lines
arch/x86/include/asm/msr-index.h +4
-0+ +#define MSR_IA32_PRED_CMD 0x00000049 +#define PRED_CMD_IBPB (1UL << 0) +The IBPB stuff should be enumerated separately. Feel free to create a separate patch for this chunk.
I think it makes sense for the patch that adds the CPU feature to also change msr-index.h.
Paolo
Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
Quoted 13 lines · Paolo Bonzini
Quoted 11 lines · Tim Chen
arch/x86/include/asm/msr-index.h line 470
Quoted 4 lines · Paolo Bonzini
arch/x86/include/asm/msr-index.h +4
-0+ +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0)Boris requested that the name for ENABLE/DISABLE to be shortened to SPEC_CTRL_{ENABLE,DISABLE}_IBRS. I'm planning to do that for the next patchset update.
Quoted 2 lines
Were you also planning to include KVM patches in your patchset sooner or later? Or would that be a separate submission?
Ashok Raj will be sending out the KVM related patches in a short while after I updated my patchset.
Tim
Andy Lutomirski
maintainerRe: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation
In reply to Tim ChenQuoted 54 lines · Tim Chen
cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature
IA32_SPEC_CTRL(0x48)IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS)If IBRS is set, near returns and near indirect jumps/calls will not allow their predicted target address to be controlled by code that executed in a less privileged prediction mode before the IBRS mode was last written with a value of 1 or on another logical processor so long as all RSB entries from the previous less privileged prediction mode are overwritten.
* Thus a near indirect jump/call/return may be affected by code in a less privileged prediction mode that executed AFTER IBRS mode was last written with a value of 1
* There is no need to clear IBRS before writing it with a value of
1. Unconditionally writing it with a value of 1 after the prediction mode change is sufficient* Note: IBRS is not required in order to isolate branch predictions for SMM or SGX enclaves
* Code executed by a sibling logical processor cannot control indirect jump/call/return predicted target when IBRS is set
* SMEP will prevent supervisor mode using RSB entries filled by user code; this can reduce the need for software to overwrite RSB entries
CPU performance could be reduced when running with IBRS set.
Signed-off-by Tim Chen <[email protected]>--- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 4 ++++ arch/x86/kernel/cpu/scattered.c | 1 + tools/arch/x86/include/asm/cpufeatures.h | 1 + 4 files changed, 7 insertions(+)arch/x86/include/asm/cpufeatures.h +1
-0@@ -209,6 +209,7 @@#define X86_FEATURE_AVX512_4FMAPS ( 7*32+17)/* AVX-512 Multiply Accumulation Single precision */ #define X86_FEATURE_MBA ( 7*32+18)/* Memory Bandwidth Allocation */ +#define X86_FEATURE_SPEC_CTRL ( 7*32+19)/* Control Speculation Control */ /* Virtualization flags: Linux defined, word 8 */ #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0)/* Intel TPR Shadow */ arch/x86/include/asm/msr-index.h +0
-0
arch/x86/include/asm/msr-index.h line 48
Quoted 7 lines
arch/x86/include/asm/msr-index.h +4
-0@@ -42,6 +42,10 @@#define MSR_PPIN_CTL 0x0000004e#define MSR_PPIN 0x0000004f +#define MSR_IA32_SPEC_CTRL 0x00000048 +#define SPEC_CTRL_FEATURE_DISABLE_IBRS (0 << 0) +#define SPEC_CTRL_FEATURE_ENABLE_IBRS (1 << 0) +
I still can't find the PDF that documents this right now, but, from memory, shouldn't the value that we write to the MSR on entry be 3, not 1? That is, isn't bit 1 STIBP and don't we want STIBP to be on? And IIRC bit 1 was ignored on non-STIBP-capable machines.
PATCH v2 2/8 x86/enter: MACROS to set/clear IBRS
In reply to Tim ChenCreate macros to control IBRS. Use these macros to enable IBRS on kernel entry paths and disable IBRS on kernel exit paths.
The registers rax, rcx and rdx are touched when controlling IBRS so they need to be saved when they can't be clobbered.
--- arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
arch/x86/entry/calling.h +74 -0
@@ -6,6 +6,8 @@#include <asm/percpu.h> #include <asm/asm-offsets.h> #include <asm/processor-flags.h> +#include <asm/msr-index.h> +#include <asm/cpufeatures.h> /*
@@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with .Lafter_call_\@: #endif .endm + +/* + * IBRS related macros + */ + +.macro PUSH_MSR_REGS + pushq %rax + pushq %rcx + pushq %rdx +.endm + +.macro POP_MSR_REGS + popq %rdx + popq %rcx + popq %rax +.endm + +.macro WRMSR_ASM msr_nr:req eax_val:req + movl \msr_nr, %ecx + movl $0, %edx + movl \eax_val, %eax +.endm + +.macro ENABLE_IBRS + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + PUSH_MSR_REGS + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS + POP_MSR_REGS +.Lskip_\@: +.endm + +.macro DISABLE_IBRS + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + PUSH_MSR_REGS + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS + POP_MSR_REGS +.Lskip_\@: +.endm + +.macro ENABLE_IBRS_CLOBBER + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS +.Lskip_\@: +.endm + +.macro DISABLE_IBRS_CLOBBER + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS +.Lskip_\@: +.endm + +.macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + movl $MSR_IA32_SPEC_CTRL, %ecx + rdmsr + movl %eax, \save_reg + + movl $0, %edx + movl $SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax + wrmsr +.Lskip_\@: +.endm + +.macro RESTORE_IBRS_CLOBBER save_reg:req + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + /* Set IBRS to the value saved in the save_reg */ + movl $MSR_IA32_SPEC_CTRL, %ecx + movl $0, %edx + movl \save_reg, %eax + wrmsr +.Lskip_\@: +.endm
Signature · 1 line
2.9.4
Borislav Petkov
maintainerRe: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
Quoted 1 line · Tim Chen
Subject: Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
Your subject needs to have a verb and not scream:
Subject: [PATCH v2 2/8] x86/entry: Add macros to set/clear IBRS
Quoted 35 lines
Create macros to control IBRS. Use these macros to enable IBRS on kernel entry paths and disable IBRS on kernel exit paths.
The registers rax, rcx and rdx are touched when controlling IBRS so they need to be saved when they can't be clobbered.
Signed-off-by Tim Chen <[email protected]>--- arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)arch/x86/entry/calling.h +9
-0@@ -6,6 +6,8 @@#include <asm/percpu.h> #include <asm/asm-offsets.h> #include <asm/processor-flags.h> +#include <asm/msr-index.h> +#include <asm/cpufeatures.h> /* @@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with .Lafter_call_\@: #endif .endm + +/* + * IBRS related macros + */ +.macro PUSH_MSR_REGS + pushq %rax + pushq %rcx
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 368
Quoted 10 lines
arch/x86/entry/calling.h +10
-0+ pushq %rdx +.endm + +.macro POP_MSR_REGS + popq %rdx + popq %rcx + popq %rax +.endm + +.macro WRMSR_ASM msr_nr:req eax_val:req
WRMSR as a name is good enough.
Also, you need edx_val:req too in case we decide to reuse that macro for something else later. Which I'm pretty sure we will, once it is out there.
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 370
Quoted 2 lines
arch/x86/entry/calling.h +2
-0+ movl \msr_nr, %ecx + movl $0, %edx
... and then
movl \edx_val, %edx
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 378
Quoted 7 lines
arch/x86/entry/calling.h +7
-0+ movl \eax_val, %eax +.endm + +.macro ENABLE_IBRS + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL +PUSH_MSR_REGS + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
This is overwriting the previous contents of the MSR. You need to read it and OR-in its bits [63:2] with SPEC_CTRL_FEATURE_ENABLE_IBRS and clear bit 0.
Unless the rest of this MSR is not going to be used for anything else. Then you're fine.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
Quoted 79 lines · Borislav Petkov
Quoted 1 line · Tim Chen
Subject: Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
Your subject needs to have a verb and not scream:
Subject: [PATCH v2 2/8] x86/entry: Add macros to set/clear IBRS
Quoted 35 lines
Create macros to control IBRS. Use these macros to enable IBRS on kernel entry paths and disable IBRS on kernel exit paths.
The registers rax, rcx and rdx are touched when controlling IBRS so they need to be saved when they can't be clobbered.
Signed-off-by Tim Chen <[email protected]>--- arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)arch/x86/entry/calling.h +9
-0@@ -6,6 +6,8 @@#include <asm/percpu.h> #include <asm/asm-offsets.h> #include <asm/processor-flags.h> +#include <asm/msr-index.h> +#include <asm/cpufeatures.h> /* @@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with .Lafter_call_\@: #endif .endm + +/* + * IBRS related macros + */ +.macro PUSH_MSR_REGS + pushq %rax + pushq %rcxarch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 368
Quoted 10 lines
arch/x86/entry/calling.h +10
-0+ pushq %rdx +.endm + +.macro POP_MSR_REGS + popq %rdx + popq %rcx + popq %rax +.endm + +.macro WRMSR_ASM msr_nr:req eax_val:reqWRMSR as a name is good enough.
Also, you need edx_val:req too in case we decide to reuse that macro for something else later. Which I'm pretty sure we will, once it is out there.
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 370
Quoted 2 lines
arch/x86/entry/calling.h +2
-0+ movl \msr_nr, %ecx + movl $0, %edx... and then
movl \edx_val, %edxarch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 378
Quoted 7 lines
arch/x86/entry/calling.h +7
-0+ movl \eax_val, %eax +.endm + +.macro ENABLE_IBRS + ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL +PUSH_MSR_REGS + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRSThis is overwriting the previous contents of the MSR. You need to read it and OR-in its bits [63:2] with
SPEC_CTRL_FEATURE_ENABLE_IBRSand clear bit 0.
Quoted 3 lines
Unless the rest of this MSR is not going to be used for anything else. Then you're fine.
Currently we are not using other bits.
When the time comes that we have other bits in this MSR used, we will change this.
Tim
Borislav Petkov
maintainerRe: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
Quoted 3 lines · Tim Chen
Currently we are not using other bits.
When the time comes that we have other bits in this MSR used, we will change this.
Then please write in a short comment above it that it is ok that the previous MSR contents get overwritten.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
In reply to Borislav PetkovQuoted 3 lines · Borislav Petkov
WRMSR as a name is good enough.
It alias with wrmsr instruction and didn't
build correctly. So I'll keep it as WRMSR_ASM.
Tim
PATCH v2 3/8 x86/enter: Use IBRS on syscall and interrupts
In reply to Tim ChenFrom: Andrea Arcangeli <[email protected]>
Set IBRS upon kernel entrance via syscall and interrupts. Clear it upon exit. IBRS protects against unsafe indirect branching predictions in the kernel.
The NMI interrupt save/restore of IBRS state was based on Andrea Arcangeli's implementation.
Here's an explanation by Dave Hansen on why we save IBRS state for NMI.
The normal interrupt code uses the 'error_entry' path which uses the Code Segment (CS) of the instruction that was interrupted to tell whether it interrupted the kernel or userspace and thus has to switch IBRS, or leave it alone.
The NMI code is different. It uses 'paranoid_entry' because it can interrupt the kernel while it is running with a userspace IBRS (and %GS and CR3) value, but has a kernel CS. If we used the same approach as the normal interrupt code, we might do the following;
SYSENTER_entry
<-------------- NMI HERE
IBRS=1 do_something() IBRS=0 SYSRET
The NMI code might notice that we are running in the kernel and decide that it is OK to skip the IBRS=1. This would leave it running unprotected with IBRS=0, which is bad.
However, if we unconditionally set IBRS=1, in the NMI, we might get the following case:
SYSENTER_entry IBRS=1 do_something() IBRS=0
<-------------- NMI HERE (set IBRS=1)
SYSRET
and we would return to userspace with IBRS=1. Userspace would run slowly until we entered and exited the kernel again.
Instead of those two approaches, we chose a third one where we simply save the IBRS value in a scratch register (%r13) and then restore that value, verbatim.
--- arch/x86/entry/entry_64.S | 23 +++++++++++++++++++++++ arch/x86/entry/entry_64_compat.S | 8 ++++++++ 2 files changed, 31 insertions(+)
arch/x86/entry/entry_64.S +23 -0
@@ -174,6 +174,8 @@ ENTRY(entry_SYSCALL_64_trampoline) /* Load the top of the task stack into RSP */ movq CPU_ENTRY_AREA_tss + TSS_sp1 + CPU_ENTRY_AREA, %rsp + /* Stack is usable, use the non-clobbering IBRS enable: */ + ENABLE_IBRS /* Start building the simulated IRET frame. */ pushq $__USER_DS /* pt_regs->ss */
@@ -217,6 +219,8 @@ ENTRY(entry_SYSCALL_64) */ movq %rsp, PER_CPU_VAR(rsp_scratch) movq PER_CPU_VAR(cpu_current_top_of_stack), %rsp + /* Stack is usable, use the non-clobbering IBRS enable: */ + ENABLE_IBRS /* Construct struct pt_regs on stack */ pushq $__USER_DS /* pt_regs->ss */
@@ -411,6 +415,7 @@ syscall_return_via_sysret: * We are on the trampoline stack. All regs except RDI are live. * We can do future final exit work right here. */ + DISABLE_IBRS SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi popq %rdi
@@ -749,6 +754,7 @@ GLOBAL(swapgs_restore_regs_and_return_to_usermode) * We can do future final exit work right here. */ + DISABLE_IBRS SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi /* Restore RDI. */
@@ -836,6 +842,14 @@ native_irq_return_ldt: SWAPGS /* to kernel GS */ SWITCH_TO_KERNEL_CR3 scratch_reg=%rdi /* to kernel CR3 */ + /* + * Normally we enable IBRS when we switch to kernel's CR3. + * But we are going to switch back to user CR3 immediately + * in this routine after fixing ESPFIX stack. There is + * no vulnerable code branching for IBRS to protect. + * We don't toggle IBRS to avoid the cost of two MSR writes. + */ + movq PER_CPU_VAR(espfix_waddr), %rdi movq %rax, (0*8)(%rdi) /* user RAX */ movq (1*8)(%rsp), %rax /* user RIP */
@@ -969,6 +983,8 @@ ENTRY(switch_to_thread_stack) SWITCH_TO_KERNEL_CR3 scratch_reg=%rdi movq %rsp, %rdi movq PER_CPU_VAR(cpu_current_top_of_stack), %rsp + /* Stack is usable, use the non-clobbering IBRS enable: */ + ENABLE_IBRS UNWIND_HINT sp_offset=16 sp_reg=ORC_REG_DI pushq 7*8(%rdi) /* regs->ss */
@@ -1271,6 +1287,7 @@ ENTRY(paranoid_entry) 1: SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg=%rax save_reg=%r14 + ENABLE_IBRS_SAVE_AND_CLOBBER save_reg=%r13d ret END(paranoid_entry)
@@ -1294,6 +1311,7 @@ ENTRY(paranoid_exit) testl %ebx, %ebx /* swapgs needed? */ jnz .Lparanoid_exit_no_swapgs TRACE_IRQS_IRETQ + RESTORE_IBRS_CLOBBER save_reg=%r13d RESTORE_CR3 scratch_reg=%rbx save_reg=%r14 SWAPGS_UNSAFE_STACK jmp .Lparanoid_exit_restore
@@ -1324,6 +1342,7 @@ ENTRY(error_entry) SWAPGS /* We have user CR3. Change to kernel CR3. */ SWITCH_TO_KERNEL_CR3 scratch_reg=%rax + ENABLE_IBRS_CLOBBER .Lerror_entry_from_usermode_after_swapgs: /* Put us onto the real thread stack. */
@@ -1371,6 +1390,7 @@ ENTRY(error_entry) */ SWAPGS SWITCH_TO_KERNEL_CR3 scratch_reg=%rax + ENABLE_IBRS_CLOBBER jmp .Lerror_entry_done .Lbstep_iret:
@@ -1385,6 +1405,7 @@ ENTRY(error_entry) */ SWAPGS SWITCH_TO_KERNEL_CR3 scratch_reg=%rax + ENABLE_IBRS /* * Pretend that the exception came from user mode: set up pt_regs
@@ -1486,6 +1507,7 @@ ENTRY(nmi) SWITCH_TO_KERNEL_CR3 scratch_reg=%rdx movq %rsp, %rdx movq PER_CPU_VAR(cpu_current_top_of_stack), %rsp + ENABLE_IBRS UNWIND_HINT_IRET_REGS base=%rdx offset=8 pushq 5*8(%rdx) /* pt_regs->ss */ pushq 4*8(%rdx) /* pt_regs->rsp */
@@ -1736,6 +1758,7 @@ end_repeat_nmi: movq $-1, %rsi call do_nmi + RESTORE_IBRS_CLOBBER save_reg=%r13d RESTORE_CR3 scratch_reg=%r15 save_reg=%r14 testl %ebx, %ebx /* swapgs needed? */
arch/x86/entry/entry_64_compat.S +8 -0
@@ -54,6 +54,7 @@ ENTRY(entry_SYSENTER_compat) SWITCH_TO_KERNEL_CR3 scratch_reg=%rsp movq PER_CPU_VAR(cpu_current_top_of_stack), %rsp + ENABLE_IBRS /* * User tracing code (ptrace or signal handlers) might assume that
@@ -224,6 +225,7 @@ GLOBAL(entry_SYSCALL_compat_after_hwframe) * preserved during the C calls inside TRACE_IRQS_OFF anyway. */ SWITCH_TO_KERNEL_CR3 scratch_reg=%rdi + ENABLE_IBRS_CLOBBER /* clobbers %rax, %rcx, %rdx */ /* * User mode is traced as though IRQs are on, and SYSENTER
@@ -240,6 +242,12 @@ GLOBAL(entry_SYSCALL_compat_after_hwframe) /* Opportunistic SYSRET */ sysret32_from_system_call: TRACE_IRQS_ON /* User mode traces as IRQs on. */ + /* + * Clobber of %rax, %rcx, %rdx is OK before register restoring. + * This is safe to do here because we have no indirect branches + * between here and the return to userspace (sysretl). + */ + DISABLE_IBRS_CLOBBER movq RBX(%rsp), %rbx /* pt_regs->rbx */ movq RBP(%rsp), %rbp /* pt_regs->rbp */ movq EFLAGS(%rsp), %r11 /* pt_regs->flags (in r11) */
Signature · 1 line
2.9.4
Re: [PATCH v2 3/8] x86/enter: Use IBRS on syscall and interrupts
Quoted 41 lines · Tim Chen
From: Andrea Arcangeli <[email protected]>
Set IBRS upon kernel entrance via syscall and interrupts. Clear it upon exit. IBRS protects against unsafe indirect branching predictions in the kernel.
The NMI interrupt save/restore of IBRS state was based on Andrea Arcangeli's implementation.
Here's an explanation by Dave Hansen on why we save IBRS state for NMI.The normal interrupt code uses the '
error_entry' path which uses the Code Segment (CS) of the instruction that was interrupted to tell whether it interrupted the kernel or userspace and thus has to switch IBRS, or leave it alone.The NMI code is different. It uses '
paranoid_entry' because it can interrupt the kernel while it is running with a userspace IBRS (and %GS and CR3) value, but has a kernel CS. If we used the same approach as the normal interrupt code, we might do the following;
SYSENTER_entry
<-------------- NMI HEREIBRS=1 do_something() IBRS=0 SYSRETThe NMI code might notice that we are running in the kernel and decide that it is OK to skip the IBRS=1. This would leave it running unprotected with IBRS=0, which is bad.
However, if we unconditionally set IBRS=1, in the NMI, we might get the following case:
SYSENTER_entry IBRS=1 do_something() IBRS=0<-------------- NMI HERE (set IBRS=1)
SYSRET
Quoted 6 lines
and we would return to userspace with IBRS=1. Userspace would run slowly until we entered and exited the kernel again.
Instead of those two approaches, we chose a third one where we simply save the IBRS value in a scratch register (%r13) and then restore that value, verbatim.
That's one helluva commit message. This is how you write commit messages!
Quoted 6 lines
Signed-off-by Andrea Arcangeli <[email protected]>Signed-off-by Tim Chen <[email protected]>--- arch/x86/entry/entry_64.S | 23 +++++++++++++++++++++++ arch/x86/entry/entry_64_compat.S | 8 ++++++++ 2 files changed, 31 insertions(+)
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.PATCH v2 4/8 x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim ChenFrom: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>
There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usage
Otherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
2. At run time
echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
The implementation was updated with input and suggestions from Andrea Arcangeli.
--- arch/x86/entry/calling.h | 42 ++++++++-- arch/x86/include/asm/spec_ctrl.h | 15 ++++ arch/x86/kernel/cpu/Makefile | 1 + arch/x86/kernel/cpu/scattered.c | 2 + arch/x86/kernel/cpu/spec_ctrl.c | 160 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 arch/x86/include/asm/spec_ctrl.h create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c
arch/x86/entry/calling.h +36 -6
@@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel is built with .endm .macro ENABLE_IBRS - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS POP_MSR_REGS + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm .macro DISABLE_IBRS - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS POP_MSR_REGS + .Lskip_\@: .endm .macro ENABLE_IBRS_CLOBBER - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm .macro DISABLE_IBRS_CLOBBER - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS + .Lskip_\@: .endm .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + movl $MSR_IA32_SPEC_CTRL, %ecx rdmsr movl %eax, \save_reg
@@ -409,15 +429,25 @@ For 32-bit we have the following conventions - kernel is built with movl $0, %edx movl $SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax wrmsr + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm .macro RESTORE_IBRS_CLOBBER save_reg:req - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ +/* Set IBRS to the value saved in the save_reg */ movl $MSR_IA32_SPEC_CTRL, %ecx movl $0, %edx movl \save_reg, %eax wrmsr + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm
arch/x86/include/asm/spec_ctrl.h new file +15 -0
@@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _ASM_X86_SPEC_CTRL_H +#define _ASM_X86_SPEC_CTRL_H + +#include <asm/msr-index.h> +#include <asm/cpufeatures.h> +#include <asm/microcode.h> + +void scan_spec_ctrl_feature (struct cpuinfo_x86 *c); +bool ibrs_inuse (void ); + +extern unsigned int dynamic_ibrs; + +#endif /* _ASM_X86_SPEC_CTRL_H */
arch/x86/kernel/cpu/Makefile +1 -0
@@ -24,6 +24,7 @@ obj-y += match.o obj-y += bugs.o obj-y += aperfmperf.o obj-y += cpuid-deps.o +obj-y += spec_ctrl.o obj-$(CONFIG_PROC_FS) += proc.o obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
arch/x86/kernel/cpu/scattered.c +2 -0
@@ -8,6 +8,7 @@#include <asm/processor.h> #include <asm/apic.h> +#include <asm/spec_ctrl.h> struct cpuid_bit {u16 feature;
@@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)if (regs[cb->reg] & (1 << cb->bit))set_cpu_cap (c, cb->feature); } +scan_spec_ctrl_feature (c); } u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
arch/x86/kernel/cpu/spec_ctrl.c new file +160 -0
160 lines not on this page · raw message
Signature · 1 line
2.9.4
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 377
Quoted 4 lines · Tim Chen
.macro
ENABLE_IBRSarch/x86/entry/calling.h +2
-1-ALTERNATIVE "jmp .Lskip_\@" ,"" ,X86_FEATURE_SPEC_CTRL +testl $1, dynamic_ibrs + jz .Lskip_\@
There was an earlier suggestion to use STATIC_JUMP_IF_... here. That's a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
We would love to do that minor really soon, but we figured everyone would rather see this version than wait for us to debug such a minor tweak.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 7 lines · Dave Hansen
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 377
Quoted 4 lines · Tim Chen
.macro
ENABLE_IBRSarch/x86/entry/calling.h +2
-1-ALTERNATIVE "jmp .Lskip_\@" ,"" ,X86_FEATURE_SPEC_CTRL +testl $1, dynamic_ibrs + jz .Lskip_\@There was an earlier suggestion to use
STATIC_JUMP_IF_... here. That's
Yes, and thanks for Cc'ing me... :/
Quoted 5 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
We would love to do that minor really soon, but we figured everyone would rather see this version than wait for us to debug such a minor tweak.
Its not minor, you need that LFENCE without it. Which makes the whole thing unconditionally expensive, which sucks.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 2 lines · Peter Zijlstra
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
The below boots, but I lack stuff to test the enabling.
arch/x86/include/asm/spec_ctrl.h +3 -11
@@ -9,9 +9,8 @@void scan_spec_ctrl_feature (struct cpuinfo_x86 *c);void rescan_spec_ctrl_feature (struct cpuinfo_x86 *c); -bool ibrs_inuse (void ); -extern unsigned int dynamic_ibrs; +DECLARE_STATIC_KEY_FALSE (ibrs_key);static inline void __disable_indirect_speculation (void ) {
@@ -31,21 +30,14 @@ static inline void __enable_indirect_spestatic inline void unprotected_speculation_begin (void ) {lockdep_assert_irqs_disabled (); -if (dynamic_ibrs) +if (static_branch_unlikely (&ibrs_key))__enable_indirect_speculation (); }static inline void unprotected_speculation_end (void ) { -if (dynamic_ibrs) { +if (static_branch_unlikely (&ibrs_key))__disable_indirect_speculation (); - }else { -/* -* rmb prevent unwanted speculation when we -* are setting IBRS -*/ -rmb (); - } }#endif /* _ASM_X86_SPEC_CTRL_H */
arch/x86/kernel/cpu/spec_ctrl.c +9 -12
@@ -7,8 +7,7 @@#include <asm/spec_ctrl.h> #include <asm/cpufeature.h> -unsigned int dynamic_ibrs__read_mostly ; -EXPORT_SYMBOL_GPL (dynamic_ibrs); +DEFINE_STATIC_KEY_FALSE (ibrs_key);enum {IBRS_DISABLED ,
@@ -27,7 +26,7 @@ DEFINE_MUTEX(spec_ctrl_mutex);static inline void set_ibrs_feature (void ) {if (!ibrs_admin_disabled) { - dynamic_ibrs =1 ; +static_branch_enable (&ibrs_key); ibrs_enabled =IBRS_ENABLED ; } }
@@ -54,12 +53,6 @@ void rescan_spec_ctrl_feature(struct cpu }EXPORT_SYMBOL_GPL (rescan_spec_ctrl_feature); -bool ibrs_inuse (void ) -{ -return ibrs_enabled ==IBRS_ENABLED ; -} -EXPORT_SYMBOL_GPL (ibrs_inuse); -static int __init noibrs (char *str) { ibrs_admin_disabled = true;
@@ -124,19 +117,23 @@ static ssize_t ibrs_enabled_write(structif (enable ==IBRS_DISABLED ) {/* disable IBRS usage */ ibrs_admin_disabled = true; - dynamic_ibrs =0 ; +static_branch_disable (&ibrs_key);spec_ctrl_flush_all_cpus (MSR_IA32_SPEC_CTRL ,SPEC_CTRL_FEATURE_DISABLE_IBRS ); }else if (enable ==IBRS_ENABLED ) {/* enable IBRS usage in kernel */ ibrs_admin_disabled = false; - dynamic_ibrs =1 ; +static_branch_enable (&ibrs_key); +/* +* This too needs an IPI to force all active CPUs into a known state. +*/ }else if (enable ==IBRS_ENABLED_USER ) {/* enable IBRS all the time in both userspace and kernel */ ibrs_admin_disabled = false; - dynamic_ibrs =0 ; +static_branch_disable (&ibrs_key); +spec_ctrl_flush_all_cpus (MSR_IA32_SPEC_CTRL ,SPEC_CTRL_FEATURE_ENABLE_IBRS ); }
arch/x86/entry/calling.h +6 -24
@@ -373,22 +373,17 @@ For 32-bit we have the following convent .endm .macro ENABLE_IBRS - testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS POP_MSR_REGS - jmp .Ldone_\@ .Lskip_\@: - lfence -.Ldone_\@: .endm .macro DISABLE_IBRS - testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
@@ -398,20 +393,15 @@ For 32-bit we have the following convent .endm .macro ENABLE_IBRS_CLOBBER - testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0 WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS - jmp .Ldone_\@ .Lskip_\@: - lfence -.Ldone_\@: .endm .macro DISABLE_IBRS_CLOBBER - testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
@@ -419,8 +409,7 @@ For 32-bit we have the following convent .endm .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req - testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0 movl $MSR_IA32_SPEC_CTRL, %ecx rdmsr
@@ -429,25 +418,18 @@ For 32-bit we have the following convent movl $0, %edx movl $SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax wrmsr - jmp .Ldone_\@ .Lskip_\@: - lfence -.Ldone_\@: .endm .macro RESTORE_IBRS_CLOBBER save_reg:req - testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0 /* Set IBRS to the value saved in the save_reg */ movl $MSR_IA32_SPEC_CTRL, %ecx movl $0, %edx movl \save_reg, %eax wrmsr - jmp .Ldone_\@ .Lskip_\@: - lfence -.Ldone_\@: .endm
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Peter Zijlstra
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
The below boots, but I lack stuff to test the enabling.
Peter,
Thanks. Will give it a spin. One other concern is if JUMP_LABEL is not configured, this may not work, and we may still need fall back to a control variable.
Tim
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 5 lines · Tim Chen
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines · Peter Zijlstra
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
Quoted 7 lines
Quoted 1 line
The below boots, but I lack stuff to test the enabling.
Peter,
Thanks. Will give it a spin. One other concern is if
JUMP_LABELis not configured, this may not work, and we may still need fall back to a control variable.
Urgh yes.. I always forget about that case. Will the retpoline crud be backported to a GCC old enough to not have asm-goto? If not, we could make all of this simply require asm-goto.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 14 lines · Peter Zijlstra
Quoted 4 lines · Tim Chen
Quoted 3 lines · Peter Zijlstra
Quoted 2 lines · Peter Zijlstra
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
Quoted 8 lines
Quoted 2 lines
The below boots, but I lack stuff to test the enabling.
Peter,
Thanks. Will give it a spin. One other concern is ifJUMP_LABELis not configured, this may not work, and we may still need fall back to a control variable.
Quoted 3 lines
Urgh yes.. I always forget about that case. Will the retpoline crud be backported to a GCC old enough to not have asm-goto? If not, we could make all of this simply require asm-goto.
I think HJ said he was planning to backport as far as 4.9. When was asm-goto added?
RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 16 lines · Peter Zijlstra
Quoted 14 lines · Tim Chen
Quoted 12 lines · Peter Zijlstra
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
The below boots, but I lack stuff to test the enabling.
Peter,
Thanks. Will give it a spin. One other concern is if
JUMP_LABELis not configured, this may not work, and we may still need fall back to a control variable.
Quoted 6 lines
Quoted 3 lines
Urgh yes.. I always forget about that case. Will the retpoline crud be backported to a GCC old enough to not have asm-goto? If not, we could make all of this simply require asm-goto.
I think HJ said he was planning to backport as far as 4.9. When was asm- goto added?
It was added to GCC 4.5.
H.J.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Peter ZijlstraQuoted 10 lines · Peter Zijlstra
Quoted 9 lines · Tim Chen
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Peter Zijlstra
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
The below boots, but I lack stuff to test the enabling.
Peter,
Quoted 7 lines
Quoted 3 lines
Thanks. Will give it a spin. One other concern is if
JUMP_LABELis not configured, this may not work, and we may still need fall back to a control variable.Urgh yes.. I always forget about that case. Will the retpoline crud be backported to a GCC old enough to not have asm-goto? If not, we could make all of this simply require asm-goto.
No, ifs and buts, really.
This wants to be a jump label and we set the requirements here. I'm tired of this completely bogus crap just to support some archaic version of GCC.
This is messy enough and no, this whole we need a control variable nonsense is just not going to happen.
Thanks,
tglx
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 19 lines · Thomas Gleixner
Quoted 10 lines · Peter Zijlstra
Quoted 9 lines · Tim Chen
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Peter Zijlstra
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
The below boots, but I lack stuff to test the enabling.
Peter,
Quoted 7 lines
Quoted 3 lines
Thanks. Will give it a spin. One other concern is if
JUMP_LABELis not configured, this may not work, and we may still need fall back to a control variable.Urgh yes.. I always forget about that case. Will the retpoline crud be backported to a GCC old enough to not have asm-goto? If not, we could make all of this simply require asm-goto.
Quoted 9 lines
No, ifs and buts, really.
This wants to be a jump label and we set the requirements here. I'm tired of this completely bogus crap just to support some archaic version of GCC.
This is messy enough and no, this whole we need a control variable nonsense is just not going to happen.
Thomas,
I'll be sending an updated patchset with boot option opt in for ibrs and leave the control varaible out. I agree that we can worry about the control variable later.
Thanks.
Tim
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 6 lines · Tim Chen
Thomas,
I'll be sending an updated patchset with boot option opt in for ibrs and leave the control varaible out. I agree that we can worry about the control variable later.
Please base this on the spectre_v2= option that's already in tip/x86/pti.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 11 lines · David Woodhouse
Quoted 6 lines · Tim Chen
Thomas,
I'll be sending an updated patchset with boot option opt in for ibrs and leave the control varaible out. I agree that we can worry about the control variable later.
Please base this on the spectre_v2= option that's already in tip/x86/pti.
Will do that for boot option.
Tim
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Peter ZijlstraQuoted 5 lines · Peter Zijlstra
Quoted 2 lines · Peter Zijlstra
Quoted 2 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
The below boots, but I lack stuff to test the enabling.
..snip..
Quoted 9 lines
arch/x86/entry/calling.h +1
-2@@ -373,22 +373,17 @@ For 32-bit we have the following convent .endm .macro ENABLE_IBRS -testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
arch/x86/entry/calling.h For 32-bit we have the following convent · line 383
Quoted 10 lines
arch/x86/entry/calling.h +0
-3PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS POP_MSR_REGS - jmp .Ldone_\@ .Lskip_\@: - lfence -.Ldone_\@: .endm
I know that this particular patchset is now obsolete as the retpoline along with stuffing the RSB half or full is the preferred way.
But I am wondering - why was the 'lfence' added in the first place if dynamic_ibrs was zero?
It certainly is not putting the speculative execution on a wild ride
like: "[tip:x86/pti] x86/retpoline: Use LFENCE instead of PAUSE in the
retpoline/RSB filling RSB macros" https://git.kernel.org/tip/2eb9137c8744f9adf1670e9aa52850948a30112b
So what was the intent behind this? Was it: "oh if we do not have IBRS let us at least add lfence on every system call, interrupt, nmi, exception, etc to do a poor man version of IBRS?"
Thank you.P.S.
My apologies if this was discussed in the prior versions of this thread. I must have missed it.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 32 lines · Konrad Rzeszutek Wilk
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Peter Zijlstra
Quoted 4 lines
a good suggestion, but we encountered some issues with it either crashing the kernel at boot or not properly turning on/off.
The below boots, but I lack stuff to test the enabling.
..snip..
Quoted 19 lines
arch/x86/entry/calling.h +0
-0@@ -373,22 +373,17 @@ For 32-bit we have the following convent.endm
.macroENABLE_IBRSarch/x86/entry/calling.h +1
-2-testl $1, dynamic_ibrs - jz .Lskip_\@ + STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
PUSH_MSR_REGS
WRMSR_ASM$MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRSPOP_MSR_REGSarch/x86/entry/calling.h +0
-1- jmp .Ldone_\@
.Lskip_\@:arch/x86/entry/calling.h +0
-2- lfence -.Ldone_\@:Quoted 1 line
.endm
I know that this particular patchset is now obsolete as the retpoline along with stuffing the RSB half or full is the preferred way.
Quoted 2 lines
But I am wondering - why was the 'lfence' added in the first place if
dynamic_ibrswas zero?
This was in the system call path, just before the jmp *sys_call_table() indirect jump. If an attacker could cause that 'jz .Lskip_\@' over the wrmsr to be predicted *taken*, speculative execution would happily proceed all the way to the indirect jump. Oops :)
Hence the 'else lfence' so that the branch-taken code path still stops to wait. Hence also the subsequent insistence on using ALTERNATIVE for it, and my paranoia about relying on GCC not missing optimisations if we're using static_cpu_has().
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim ChenQuoted 20 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usageOtherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
2. At run time
echo 0 >/sys/kernel/debug/x86/ibrs_enabledwill turn off IBRS
echo 1 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in kernel echo 2 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in both userspace and kernelThe implementation was updated with input and suggestions from Andrea Arcangeli.
Signed-off-by Tim Chen <[email protected]>---
<snip>
arch/x86/include/asm/spec_ctrl.h line 1
Quoted 5 lines
index 0000000..4fda38b
arch/x86/include/asm/spec_ctrl.h new file +1
-0@@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */
Thank you for these markings.
Quoted 4 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +1
-0@@ -0,0 +1,160 @@ +/* SPDX-License-Identifier: GPL-2.0 */
arch/x86/kernel/cpu/spec_ctrl.c line 11
Quoted 10 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +10
-0+ +#include <linux/mutex.h> +#include <linux/debugfs.h> +#include <linux/uaccess.h> + +#include <asm/spec_ctrl.h> +#include <asm/cpufeature.h> + +unsigned int dynamic_ibrs__read_mostly ; +EXPORT_SYMBOL_GPL (dynamic_ibrs);
What kernel module needs this symbol? A symbol can be "global" and not be exported, those are two different things.
And again, horrible name for a global symbol, if this really is being exported to modules (i.e. the world.)
arch/x86/kernel/cpu/spec_ctrl.c line 14
Quoted 3 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +3
-0+ +enum { +IBRS_DISABLED ,
As you are making a user/kernel API here, shouldn't you enforce the values this enum has "just to be sure"?
arch/x86/kernel/cpu/spec_ctrl.c line 20
Quoted 6 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +6
-0+/* in host kernel, disabled in guest and userland */ +IBRS_ENABLED , +/* in host kernel and host userland, disabled in guest */ +IBRS_ENABLED_USER , + IBRS_MAX = IBRS_ENABLED_USER, +};
Also, this should be documented somewhere, Documentation/ABI/ perhaps?
arch/x86/kernel/cpu/spec_ctrl.c line 25
Quoted 5 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +5
-0+static unsigned int ibrs_enabled; +static bool ibrs_admin_disabled; + +/* mutex to serialize IBRS control changes */ +DEFINE_MUTEX (spec_ctrl_mutex);
static?
arch/x86/kernel/cpu/spec_ctrl.c line 36
Quoted 11 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +11
-0+ +void scan_spec_ctrl_feature (struct cpuinfo_x86 *c) +{ +if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) { +if (!ibrs_admin_disabled) { + dynamic_ibrs =1 ; + ibrs_enabled =IBRS_ENABLED ; + } + } +} +EXPORT_SYMBOL_GPL (scan_spec_ctrl_feature);
Again, what module needs this?
Same for all the exports in this file, and again, if they are needed in modules, you need to get a better naming scheme. I'm not trying to bikeshed, it really matters, our global symbol namespace is a mess, don't make it worse :)
thanks,
greg k-h
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 77 lines · Greg KH
Quoted 20 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usageOtherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
2. At run time
echo 0 >/sys/kernel/debug/x86/ibrs_enabledwill turn off IBRS
echo 1 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in kernel echo 2 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in both userspace and kernelThe implementation was updated with input and suggestions from Andrea Arcangeli.
Signed-off-by Tim Chen <[email protected]>---
<snip>
arch/x86/include/asm/spec_ctrl.h line 1
Quoted 5 lines
index 0000000..4fda38b
arch/x86/include/asm/spec_ctrl.h new file +1
-0@@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ Thank you for these markings.
Quoted 4 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +1
-0@@ -0,0 +1,160 @@ +/* SPDX-License-Identifier: GPL-2.0 */ arch/x86/kernel/cpu/spec_ctrl.c line 11
Quoted 10 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +10
-0+ +#include <linux/mutex.h> +#include <linux/debugfs.h> +#include <linux/uaccess.h> + +#include <asm/spec_ctrl.h> +#include <asm/cpufeature.h> + +unsigned int dynamic_ibrs__read_mostly ; +EXPORT_SYMBOL_GPL (dynamic_ibrs);What kernel module needs this symbol? A symbol can be "global" and not be exported, those are two different things.
And again, horrible name for a global symbol, if this really is being exported to modules (i.e. the world.)
arch/x86/kernel/cpu/spec_ctrl.c line 14
Quoted 3 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +3
-0+ +enum { +IBRS_DISABLED ,As you are making a user/kernel API here, shouldn't you enforce the values this enum has "just to be sure"?
arch/x86/kernel/cpu/spec_ctrl.c line 20
Quoted 6 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +6
-0+/* in host kernel, disabled in guest and userland */ +IBRS_ENABLED , +/* in host kernel and host userland, disabled in guest */ +IBRS_ENABLED_USER , + IBRS_MAX = IBRS_ENABLED_USER, +};Also, this should be documented somewhere,
Documentation/ABI/perhaps?arch/x86/kernel/cpu/spec_ctrl.c line 25
Quoted 5 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +5
-0+static unsigned int ibrs_enabled; +static bool ibrs_admin_disabled; + +/* mutex to serialize IBRS control changes */ +DEFINE_MUTEX (spec_ctrl_mutex);static?
Quoted 13 lines
arch/x86/kernel/cpu/spec_ctrl.c line 36
Quoted 11 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +11
-0+ +void scan_spec_ctrl_feature (struct cpuinfo_x86 *c) +{ +if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) { +if (!ibrs_admin_disabled) { + dynamic_ibrs =1 ; + ibrs_enabled =IBRS_ENABLED ; + } + } +} +EXPORT_SYMBOL_GPL (scan_spec_ctrl_feature);Again, what module needs this?
Right now no module will need it. I'll get rid of it.
Quoted 5 lines
Same for all the exports in this file, and again, if they are needed in modules, you need to get a better naming scheme. I'm not trying to bikeshed, it really matters, our global symbol namespace is a mess, don't make it worse :)
Noted. I'll add spec_ctrl in front of those.
Quoted 5 lines
thanks,
greg k-h
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 12 lines · Tim Chen
Quoted 9 lines · Greg KH
Quoted 8 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usage
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Otherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
2. At run time
echo 0 >/sys/kernel/debug/x86/ibrs_enabledwill turn off IBRS
echo 1 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in kernel echo 2 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in both userspace and kernel
This is going to create headaches in the future.
That is future CPUs there will be no need for this MSR nor retpoline as the CPUs will observe correctness when switching .. rings/vm-exits/etc and I would assume that 'ibrs_enabled' will return 0.
And that will make folks scared and run to support/Intel with complaints.
Furthmore with the 'retpoline' work you can disable IBRS and instead use 'retpoline's as mitigation - and again the 'ibrs_enabled' is now zero. Cue in horde of customers calling support.
Would it be better to have an global /sys/../spectre_resistent instead of these 'well, check if the repoline sysfs is enabled, or if that is not, then look at the cpuid flags'.
It would be good to have this future proof.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 36 lines · Konrad Rzeszutek Wilk
Quoted 12 lines · Tim Chen
Quoted 9 lines · Greg KH
Quoted 8 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usageQuoted 8 lines
Quoted 8 lines
Quoted 8 lines
Otherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
2. At run time
echo 0 >/sys/kernel/debug/x86/ibrs_enabledwill turn off IBRS
echo 1 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in kernel echo 2 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in both userspace and kernelThis is going to create headaches in the future.
That is future CPUs there will be no need for this MSR nor retpoline as the CPUs will observe correctness when switching .. rings/vm-exits/etc and I would assume that '
ibrs_enabled' will return 0.And that will make folks scared and run to support/Intel with complaints.
Furthmore with the 'retpoline' work you can disable IBRS and instead use 'retpoline's as mitigation - and again the '
ibrs_enabled' is now zero. Cue in horde of customers calling support.
Quoted 5 lines
Would it be better to have an global
/sys/../spectre_resistentinstead of these 'well, check if the repoline sysfs is enabled, or if that is not, then look at the cpuid flags'.It would be good to have this future proof.
It's a debugfs api, it can be changed at any time, to be anything we want, and all is fine :)
Let's get this all working first please, and then a "real" api can be designed and implemented to please everyone.
thanks,
greg k-h
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim ChenQuoted 44 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usageOtherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
2. At run time
echo 0 >/sys/kernel/debug/x86/ibrs_enabledwill turn off IBRS
echo 1 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in kernel echo 2 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in both userspace and kernelThe implementation was updated with input and suggestions from Andrea Arcangeli.
Signed-off-by Tim Chen <[email protected]>--- arch/x86/entry/calling.h | 42 ++++++++-- arch/x86/include/asm/spec_ctrl.h | 15 ++++ arch/x86/kernel/cpu/Makefile | 1 + arch/x86/kernel/cpu/scattered.c | 2 + arch/x86/kernel/cpu/spec_ctrl.c | 160 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 arch/x86/include/asm/spec_ctrl.h create mode 100644 arch/x86/kernel/cpu/spec_ctrl.carch/x86/entry/calling.h +3
-1@@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel is built with .endm .macro ENABLE_IBRS - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS POP_MSR_REGS
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 390
Quoted 10 lines
arch/x86/entry/calling.h +5
-1+ jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm .macro DISABLE_IBRS -ALTERNATIVE "jmp .Lskip_\@" ,"" ,X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs
On every system call we end up hammering on this 'dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.
Would it make sense for this to be per-cpu?
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 7 lines · Konrad Rzeszutek Wilk
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.Would it make sense for this to be per-cpu?
It's probably better to either just make it __read_mostly or get the static branches that folks were suggesting actually working.
RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 10 lines
Quoted 7 lines
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.Would it make sense for this to be per-cpu?
It's probably better to either just make it
__read_mostlyor get the static branches that folks were suggesting actually working.
I still wonder if this isn't just better as a boot command line
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 6 lines · Arjan
Quoted 6 lines
Quoted 6 lines
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Would it make sense for this to be per-cpu?
It's probably better to either just make it
__read_mostlyor get the static branches that folks were suggesting actually working.I still wonder if this isn't just better as a boot command line
It's simpler that way. But, ideally, we want to make it runtime switchable to match the implementation in the distros.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 9 lines · Dave Hansen
Quoted 8 lines · Arjan
Quoted 8 lines
Quoted 7 lines
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.Would it make sense for this to be per-cpu?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
It's probably better to either just make it
__read_mostlyor get the static branches that folks were suggesting actually working.I still wonder if this isn't just better as a boot command line
It's simpler that way. But, ideally, we want to make it runtime switchable to match the implementation in the distros.
Stop this silly argument please. The distros shipped lots of crap which we dont want to have at all.
I told you folks yesterday what I want to see and the sysctl thing is the least on that list and it's not needed for getting the important thing - the protection - to work.
Can we pretty please do the basics and worry about that sysctl or whatever people have on their wishlist once the dust settled.
Thanks,
tglx
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 19 lines · Thomas Gleixner
Quoted 9 lines · Dave Hansen
Quoted 8 lines · Arjan
Quoted 8 lines
Quoted 7 lines
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.Would it make sense for this to be per-cpu?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
It's probably better to either just make it
__read_mostlyor get the static branches that folks were suggesting actually working.I still wonder if this isn't just better as a boot command line
It's simpler that way. But, ideally, we want to make it runtime switchable to match the implementation in the distros.
Quoted 6 lines
Stop this silly argument please. The distros shipped lots of crap which we dont want to have at all.
I told you folks yesterday what I want to see and the sysctl thing is the least on that list and it's not needed for getting the important thing - the protection - to work.
I agree. But this is what customers are told to inspect to see if they are impacted. And if in the future versions this goes away or such - they will freak out and cause needless escalations.
Quoted 8 lines
Can we pretty please do the basics and worry about that sysctl or whatever people have on their wishlist once the dust settled.
Thanks,
tglx
RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 3 lines
I agree. But this is what customers are told to inspect to see if they are impacted. And if in the future versions this goes away or such - they will freak out and cause needless escalations.
it's a mistake though... retpoline is what people should be using .... ... and only in super exception cases should IBRS even be considered
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 7 lines · Arjan
Quoted 3 lines
I agree. But this is what customers are told to inspect to see if they are impacted. And if in the future versions this goes away or such - they will freak out and cause needless escalations.
it's a mistake though... retpoline is what people should be using .... ... and only in super exception cases should IBRS even be considered
Like on certain Skylake and Broadwell where using the retpoline won't suffice?
As you can imagine having an heuristic that will figure out if:
- The CPU is doing the correct thing, retpoline or IBRS is not needed at all.
- The CPU can do retpoline, use that instead.
- The CPU is unsafe to do retpoline, use IBRS.
- The CPU can do retpoline, but MSRS are faster (is that even the case?)
- The CPU hasn't been updated, the retpolines are unsafe, the only solution is to buy a new CPU.
And have all of this nicely rolled out to customers in an 'sysfs' that will tell the customers - yes your are safe.
All I m saying is that the 'ibrs_enabled' is a bad name, it should be something else altogether.
RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 5 lines
Quoted 3 lines
it's a mistake though... retpoline is what people should be using .... ... and only in super exception cases should IBRS even be considered
Like on certain Skylake and Broadwell where using the retpoline won't suffice?
skylake is a bit more complex but that was discussed in earlier emails where a few more things are needed. No idea what you're inventing on broadwell where you would use ibrs but not retpoline
Quoted 1 line
- The CPU can do retpoline, but MSRS are faster (is that even the case?)
that is never the case
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Konrad Rzeszutek WilkQuoted 23 lines · Konrad Rzeszutek Wilk
Quoted 22 lines · Thomas Gleixner
Quoted 9 lines · Dave Hansen
Quoted 8 lines · Arjan
Quoted 8 lines
Quoted 7 lines
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.Would it make sense for this to be per-cpu?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
It's probably better to either just make it
__read_mostlyor get the static branches that folks were suggesting actually working.I still wonder if this isn't just better as a boot command line
It's simpler that way. But, ideally, we want to make it runtime switchable to match the implementation in the distros.
Stop this silly argument please. The distros shipped lots of crap which we dont want to have at all.
Quoted 7 lines
Quoted 3 lines
I told you folks yesterday what I want to see and the sysctl thing is the least on that list and it's not needed for getting the important thing - the protection - to work.
I agree. But this is what customers are told to inspect to see if they are impacted. And if in the future versions this goes away or such - they will freak out and cause needless escalations.
That's the result of distros cramming stuff into their kernels without talking to us. It's their problem to explain that their customers.
We can talks about the sysctl _AFTER_ fixing the real issues.
Thanks,
tglx
Is: Linus, name for 'spectre' variable. Was:Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 28 lines · Thomas Gleixner
Quoted 27 lines · Konrad Rzeszutek Wilk
Quoted 25 lines · Thomas Gleixner
Quoted 9 lines · Dave Hansen
Quoted 8 lines · Arjan
Quoted 8 lines
Quoted 7 lines
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.Would it make sense for this to be per-cpu?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
It's probably better to either just make it
__read_mostlyor get the static branches that folks were suggesting actually working.I still wonder if this isn't just better as a boot command line
It's simpler that way. But, ideally, we want to make it runtime switchable to match the implementation in the distros.
Stop this silly argument please. The distros shipped lots of crap which we dont want to have at all.
I told you folks yesterday what I want to see and the sysctl thing is the least on that list and it's not needed for getting the important thing - the protection - to work.
Quoted 6 lines
Quoted 3 lines
I agree. But this is what customers are told to inspect to see if they are impacted. And if in the future versions this goes away or such - they will freak out and cause needless escalations.
That's the result of distros cramming stuff into their kernels without talking to us. It's their problem to explain that their customers.
I am trying to resolve this now for those distros's that haven't crammed these patches in yet and get a resolution on the naming _now_ so they can update it to have the right name from the gecko for the future.
Lets rope in the boss.
Quoted 2 lines
We can talks about the sysctl _AFTER_ fixing the real issues.
Perhaps we can delegate this to Linus.
Quoted 4 lines
Thanks,
tglx
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Dave HansenQuoted 7 lines · Dave Hansen
Quoted 6 lines · Konrad Rzeszutek Wilk
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.
Quoted 5 lines
Quoted 1 line
Would it make sense for this to be per-cpu?
It's probably better to either just make it
__read_mostlyor get the static branches that folks were suggesting actually working.
dynamic_ibrs is indeed declared __read_mostly
+unsigned int dynamic_ibrs __read_mostly;
+EXPORT_SYMBOL_GPL(dynamic_ibrs);
Tim
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Konrad Rzeszutek WilkQuoted 6 lines · Konrad Rzeszutek Wilk
Quoted 5 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>
Quoted 6 lines
Review
Quoted 3 lines
.macro
DISABLE_IBRS(quoted diff) +1
-1- ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrsOn every system call we end up hammering on this '
dynamic_ibrs' variable. And it looks like it can be flipped via the IPI mechanism.
On system call, we read dynamic_ibrs value (not change it) and flip the IBRS msr only if it dynamic_ibrs is true.
We only do global change to all the IBRS msrs on all cpus during the admin request to change its value, serialized by spec_ctrl_mutex. Before we do that, we set dynamic_ibrs to 0, so each cpu no longer do any change to IBRS. Then the IPI happens to update the IBRS MSR values.
Quoted 3 lines
Would it make sense for this to be per-cpu?
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim Chenarch/x86/kernel/cpu/spec_ctrl.c line 29
Quoted 3 lines · Tim Chen
arch/x86/kernel/cpu/spec_ctrl.c new file +3
-0+void scan_spec_ctrl_feature (struct cpuinfo_x86 *c) +{ +if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) {
We should test X86_BUG_SPECTRE_V1 here too before default enabling this, no?
Quoted 6 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +6
-0+if (!ibrs_admin_disabled) { + dynamic_ibrs =1 ; + ibrs_enabled =IBRS_ENABLED ; + } + } +}
RE: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 4 lines
arch/x86/kernel/cpu/spec_ctrl.c line 29
Quoted 1 line
arch/x86/kernel/cpu/spec_ctrl.c new file +1
-0+if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) {We should test
X86_BUG_SPECTRE_V1here too before default enabling this, no?
we shouldn't default enable this.
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 8 lines · Arjan
Quoted 4 lines
arch/x86/kernel/cpu/spec_ctrl.c line 29
Quoted 1 line
arch/x86/kernel/cpu/spec_ctrl.c new file +1
-0+if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) {We should test
X86_BUG_SPECTRE_V1here too before default enabling this, no?we shouldn't default enable this.
Patch 7 disables ibrs if retpoline is in use.
Tim
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim Chenarch/x86/kernel/cpu/spec_ctrl.c line 59
Quoted 7 lines · Tim Chen
arch/x86/kernel/cpu/spec_ctrl.c new file +6
-0+bool ibrs_inuse (void ); +bool ibrs_inuse (void ) +{ +return ibrs_enabled ==IBRS_ENABLED ; +} +EXPORT_SYMBOL_GPL (ibrs_inuse);
Doesn't appear to actually be used anywhere...
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim ChenQuoted 44 lines · Tim Chen
arch/x86/kernel/cpu/spec_ctrl.c new file +44
-0+static void spec_ctrl_flush_all_cpus (u32 msr_nr,u64 val) +{ +int cpu; + +get_online_cpus (); +for_each_online_cpu (cpu) +wrmsrl_on_cpu (cpu, msr_nr, val); +put_online_cpus (); +} + +static ssize_t ibrs_enabled_write (struct file *file, +const char __user *user_buf, +size_t count,loff_t *ppos) +{ +char buf[32 ]; +ssize_t len; +unsigned int enable; + + len =min (count,sizeof (buf) -1 ); +if (copy_from_user (buf, user_buf, len)) +return -EFAULT ; + + buf[len] ='\0' ; +if (kstrtouint (buf,0 , &enable)) +return -EINVAL ; + +if (enable >IBRS_MAX ) +return -EINVAL ; + +if (!boot_cpu_has (X86_FEATURE_SPEC_CTRL )) { + ibrs_enabled =IBRS_DISABLED ; +return -EINVAL ; + } + +mutex_lock (&spec_ctrl_mutex); + +if (enable ==IBRS_DISABLED ) { +/* disable IBRS usage */ + ibrs_admin_disabled = true; + dynamic_ibrs =0 ; +spec_ctrl_flush_all_cpus (MSR_IA32_SPEC_CTRL , +SPEC_CTRL_FEATURE_DISABLE_IBRS ); + + }else if (enable ==IBRS_ENABLED ) {
arch/x86/kernel/cpu/spec_ctrl.c line 139
Quoted 10 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +10
-0+/* enable IBRS usage in kernel */ + ibrs_admin_disabled = false; + dynamic_ibrs =1 ; + + }else if (enable ==IBRS_ENABLED_USER ) { +/* enable IBRS all the time in both userspace and kernel */ + ibrs_admin_disabled = false; + dynamic_ibrs =0 ; +spec_ctrl_flush_all_cpus (MSR_IA32_SPEC_CTRL , +SPEC_CTRL_FEATURE_ENABLE_IBRS );
So what about the CPUs that were offline when you did this?
Quoted 7 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +7
-0+ } + + ibrs_enabled = enable; + +mutex_unlock (&spec_ctrl_mutex); +return count; +}
Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim ChenQuoted 27 lines · Tim Chen
arch/x86/kernel/cpu/spec_ctrl.c new file +27
-0+static ssize_t ibrs_enabled_write (struct file *file, +const char __user *user_buf, +size_t count,loff_t *ppos) +{ +char buf[32 ]; +ssize_t len; +unsigned int enable; + + len =min (count,sizeof (buf) -1 ); +if (copy_from_user (buf, user_buf, len)) +return -EFAULT ; + + buf[len] ='\0' ; +if (kstrtouint (buf,0 , &enable)) +return -EINVAL ; + +if (enable >IBRS_MAX ) +return -EINVAL ; + +if (!boot_cpu_has (X86_FEATURE_SPEC_CTRL )) { + ibrs_enabled =IBRS_DISABLED ; +return -EINVAL ; + } + +mutex_lock (&spec_ctrl_mutex); + +if (enable ==IBRS_DISABLED ) {
arch/x86/kernel/cpu/spec_ctrl.c line 132
Quoted 10 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +10
-0+/* disable IBRS usage */ + ibrs_admin_disabled = true; + dynamic_ibrs =0 ; +spec_ctrl_flush_all_cpus (MSR_IA32_SPEC_CTRL , +SPEC_CTRL_FEATURE_DISABLE_IBRS ); + + }else if (enable ==IBRS_ENABLED ) { +/* enable IBRS usage in kernel */ + ibrs_admin_disabled = false; + dynamic_ibrs =1 ;
I think you need to do:
spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
here as well, to force all CPUs into a known state.
Quoted 14 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +14
-0+ + }else if (enable ==IBRS_ENABLED_USER ) { +/* enable IBRS all the time in both userspace and kernel */ + ibrs_admin_disabled = false; + dynamic_ibrs =0 ; +spec_ctrl_flush_all_cpus (MSR_IA32_SPEC_CTRL , +SPEC_CTRL_FEATURE_ENABLE_IBRS ); + } + + ibrs_enabled = enable; + +mutex_unlock (&spec_ctrl_mutex); +return count; +}
Borislav Petkov
maintainerRe: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
In reply to Tim ChenQuoted 2 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>
There's Co-Developed-by for this:
- Co-Developed-by: states that the patch was also created by another developer along with the original author. This is useful at times when multiple people work on a single patch. Note, this person also needs to have a
Signed-off-by: line in the patch as well.
Quoted 7 lines
There are 2 ways to control IBRS
1. At boot time
noibrs kernel boot parameter will disable IBRS usageOtherwise if the above parameters are not specified, the system will enable ibrs and ibpb usage if the cpu supports it.
s/cpu/CPU/
Quoted 101 lines
2. At run time
echo 0 >/sys/kernel/debug/x86/ibrs_enabledwill turn off IBRS
echo 1 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in kernel echo 2 >/sys/kernel/debug/x86/ibrs_enabledwill turn on IBRS in both userspace and kernelThe implementation was updated with input and suggestions from Andrea Arcangeli.
Signed-off-by Tim Chen <[email protected]>--- arch/x86/entry/calling.h | 42 ++++++++-- arch/x86/include/asm/spec_ctrl.h | 15 ++++ arch/x86/kernel/cpu/Makefile | 1 + arch/x86/kernel/cpu/scattered.c | 2 + arch/x86/kernel/cpu/spec_ctrl.c | 160 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 arch/x86/include/asm/spec_ctrl.h create mode 100644 arch/x86/kernel/cpu/spec_ctrl.carch/x86/entry/calling.h +32
-6@@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel is built with .endm .macro ENABLE_IBRS - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS POP_MSR_REGS + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm .macro DISABLE_IBRS - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + PUSH_MSR_REGS WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS POP_MSR_REGS + .Lskip_\@: .endm .macro ENABLE_IBRS_CLOBBER - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm .macro DISABLE_IBRS_CLOBBER - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS + .Lskip_\@: .endm .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ + movl $MSR_IA32_SPEC_CTRL, %ecx rdmsr movl %eax, \save_reg@@ -409,15 +429,25 @@ For 32-bit we have the following conventions - kernel is built with movl $0, %edx movl $SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax wrmsr + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm .macro RESTORE_IBRS_CLOBBER save_reg:req - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL + testl $1, dynamic_ibrs + jz .Lskip_\@ +/* Set IBRS to the value saved in the save_reg */
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 453
Quoted 10 lines
arch/x86/entry/calling.h +4
-0movl $MSR_IA32_SPEC_CTRL, %ecx movl $0, %edx movl \save_reg, %eax wrmsr + jmp .Ldone_\@ + .Lskip_\@: + lfence +.Ldone_\@: .endm
Why not put all macros in spec_ctrl.h ?
Quoted 6 lines
arch/x86/include/asm/spec_ctrl.h new file +0
-0
arch/x86/include/asm/spec_ctrl.h line 8
Quoted 8 lines
arch/x86/include/asm/spec_ctrl.h new file +8
-0@@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _ASM_X86_SPEC_CTRL_H +#define _ASM_X86_SPEC_CTRL_H + +#include <asm/msr-index.h> +#include <asm/cpufeatures.h> +#include <asm/microcode.h>
Left over include from a previous version.
Instead, you need
#include <linux/cpu.h>
in spec_ctrl.c for get/put_online_cpus().
Quoted 31 lines
arch/x86/include/asm/spec_ctrl.h new file +6
-0+void scan_spec_ctrl_feature (struct cpuinfo_x86 *c); +bool ibrs_inuse (void ); + +extern unsigned int dynamic_ibrs; + +#endif /* _ASM_X86_SPEC_CTRL_H */ arch/x86/kernel/cpu/Makefile +1
-0@@ -24,6 +24,7 @@ obj-y += match.o obj-y += bugs.o obj-y += aperfmperf.o obj-y += cpuid-deps.o +obj-y += spec_ctrl.o obj-$(CONFIG_PROC_FS) += proc.o obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.oarch/x86/kernel/cpu/scattered.c +1
-0@@ -8,6 +8,7 @@#include <asm/processor.h> #include <asm/apic.h> +#include <asm/spec_ctrl.h> struct cpuid_bit {u16 feature;
arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 61
Quoted 4 lines
arch/x86/kernel/cpu/scattered.c +1
-0@@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)if (regs[cb->reg] & (1 << cb->bit))set_cpu_cap (c, cb->feature); } +scan_spec_ctrl_feature (c);
Hell no!
This function is only for setting the feature bits.
Quoted 7 lines
arch/x86/kernel/cpu/scattered.c +0
-0u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,arch/x86/kernel/cpu/spec_ctrl.c new file +0
-0
arch/x86/kernel/cpu/spec_ctrl.c line 10
Quoted 10 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +10
-0@@ -0,0 +1,160 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <linux/mutex.h> +#include <linux/debugfs.h> +#include <linux/uaccess.h> + +#include <asm/spec_ctrl.h> +#include <asm/cpufeature.h> + +unsigned int dynamic_ibrs__read_mostly ;
WTH is dynamic_ibrs?
arch/x86/kernel/cpu/spec_ctrl.c line 22
Quoted 12 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +12
-0+EXPORT_SYMBOL_GPL (dynamic_ibrs); + +enum { +IBRS_DISABLED , +/* in host kernel, disabled in guest and userland */ +IBRS_ENABLED , +/* in host kernel and host userland, disabled in guest */ +IBRS_ENABLED_USER , +IBRS_MAX =IBRS_ENABLED_USER , +}; +static unsigned int ibrs_enabled; +static bool ibrs_admin_disabled;
Srsly?!
That's *three* variables controlling IBRS. This needs simplification.
arch/x86/kernel/cpu/spec_ctrl.c line 25
Quoted 3 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +3
-0+ +/* mutex to serialize IBRS control changes */ +DEFINE_MUTEX (spec_ctrl_mutex);
static
arch/x86/kernel/cpu/spec_ctrl.c line 29
Quoted 3 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +3
-0+void scan_spec_ctrl_feature (struct cpuinfo_x86 *c) +{ +if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) {
What is !c->cpu_index? Checking whether this is the BSP? What for?
Quoted 19 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +19
-0+if (!ibrs_admin_disabled) { + dynamic_ibrs =1 ; + ibrs_enabled =IBRS_ENABLED ; + } + } +} +EXPORT_SYMBOL_GPL (scan_spec_ctrl_feature); + +/* +* Used after boot phase to rescan spec_ctrl feature, +* serialize scan with spec_ctrl_mutex. +*/ +void rescan_spec_ctrl_feature (struct cpuinfo_x86 *c) +{ +mutex_lock (&spec_ctrl_mutex); +if (boot_cpu_has (X86_FEATURE_SPEC_CTRL )) { +if (!ibrs_admin_disabled) { + dynamic_ibrs =1 ; + ibrs_enabled =IBRS_ENABLED ;
arch/x86/kernel/cpu/spec_ctrl.c line 58
Quoted 10 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +10
-0+ } + } +mutex_unlock (&spec_ctrl_mutex); +} +EXPORT_SYMBOL_GPL (rescan_spec_ctrl_feature); + +bool ibrs_inuse (void ) +{ +return ibrs_enabled ==IBRS_ENABLED ; +}
So from looking at this, you can have a single ibrs_state variable and set bits in it and have *all* possible settings you wanna have.
arch/x86/kernel/cpu/spec_ctrl.c line 59
Quoted 1 line
arch/x86/kernel/cpu/spec_ctrl.c new file +1
-0+EXPORT_SYMBOL_GPL (ibrs_inuse);
That naming is not really correct: otherwise IBRS_ENABLED_USER means ibrs is *not* in use.
Quoted 46 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +46
-0+ +static int __init noibrs (char *str) +{ + ibrs_admin_disabled = true; + ibrs_enabled =IBRS_DISABLED ; + +return 0 ; +} +early_param ("noibrs" , noibrs); + +static ssize_t __enabled_read (struct file *file,char __user *user_buf, +size_t count,loff_t *ppos,unsigned int *field) +{ +char buf[32 ]; +unsigned int len; + + len =sprintf (buf,"%d\n" ,READ_ONCE (*field)); +return simple_read_from_buffer (user_buf, count, ppos, buf, len); +} + +static ssize_t ibrs_enabled_read (struct file *file,char __user *user_buf, +size_t count,loff_t *ppos) +{ +return __enabled_read (file, user_buf, count, ppos, &ibrs_enabled); +} + +static void spec_ctrl_flush_all_cpus (u32 msr_nr,u64 val) +{ +int cpu; + +get_online_cpus (); +for_each_online_cpu (cpu) +wrmsrl_on_cpu (cpu, msr_nr, val); +put_online_cpus (); +} + +static ssize_t ibrs_enabled_write (struct file *file, +const char __user *user_buf, +size_t count,loff_t *ppos) +{ +char buf[32 ]; +ssize_t len; +unsigned int enable; + + len =min (count,sizeof (buf) -1 ); +if (copy_from_user (buf, user_buf, len))
arch/x86/kernel/cpu/spec_ctrl.c line 115
Quoted 10 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +10
-0+return -EFAULT ; + + buf[len] ='\0' ; +if (kstrtouint (buf,0 , &enable)) +return -EINVAL ; + +if (enable >IBRS_MAX ) +return -EINVAL ; + +if (!boot_cpu_has (X86_FEATURE_SPEC_CTRL )) {
That capability check needs to happen first: no need to do anything if the CPU doesn't have IBRS.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
Quoted 11 lines · Borislav Petkov
Quoted 2 lines · Tim Chen
From: Tim Chen <[email protected]>
From: Andrea Arcangeli <[email protected]>There's Co-Developed-by for this:
- Co-Developed-by: states that the patch was also created by another developer along with the original author. This is useful at times when multiple people work on a single patch. Note, this person also needs to have a
Signed-off-by: line in the patch as well.
Thanks. Will do.
Quoted 6 lines
arch/x86/entry/calling.h For 32-bit we have the following conventions - kernel is built with · line 387
Quoted 4 lines
.Lskip_\@:
arch/x86/entry/calling.h +2
-0+ lfence +.Ldone_\@: .endmWhy not put all macros in
spec_ctrl.h?
There were a previous discussion thread in v1 patch with Peter Z and Dave. Peter and Dave prefer that all these entrance macros be consolidated in calling.h
Quoted 75 lines
Quoted 7 lines
arch/x86/include/asm/spec_ctrl.h new file +0
-0arch/x86/include/asm/spec_ctrl.h line 8
Quoted 8 lines
arch/x86/include/asm/spec_ctrl.h new file +8
-0@@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _ASM_X86_SPEC_CTRL_H +#define _ASM_X86_SPEC_CTRL_H + +#include <asm/msr-index.h> +#include <asm/cpufeatures.h> +#include <asm/microcode.h> Left over include from a previous version.
Instead, you need
#include <
linux/cpu.h>in
spec_ctrl.cfor get/put_online_cpus().Quoted 31 lines
arch/x86/include/asm/spec_ctrl.h new file +6
-0+void scan_spec_ctrl_feature (struct cpuinfo_x86 *c); +bool ibrs_inuse (void ); + +extern unsigned int dynamic_ibrs; + +#endif /* _ASM_X86_SPEC_CTRL_H */ arch/x86/kernel/cpu/Makefile +1
-0@@ -24,6 +24,7 @@ obj-y += match.o obj-y += bugs.o obj-y += aperfmperf.o obj-y += cpuid-deps.o +obj-y += spec_ctrl.o obj-$(CONFIG_PROC_FS) += proc.o obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.oarch/x86/kernel/cpu/scattered.c +1
-0@@ -8,6 +8,7 @@#include <asm/processor.h> #include <asm/apic.h> +#include <asm/spec_ctrl.h> struct cpuid_bit {u16 feature;arch/x86/kernel/cpu/scattered.c void init_scattered_cpuid_features(struct cpuinfo_x86 *c) · line 61
Quoted 4 lines
arch/x86/kernel/cpu/scattered.c +1
-0@@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)if (regs[cb->reg] & (1 << cb->bit))set_cpu_cap (c, cb->feature); } +scan_spec_ctrl_feature (c);Hell no!
This function is only for setting the feature bits.
Quoted 11 lines
arch/x86/kernel/cpu/scattered.c +0
-0u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,arch/x86/kernel/cpu/spec_ctrl.c new file +4
-0@@ -0,0 +1,160 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <linux/mutex.h> +#include <linux/debugfs.h>
Quoted 8 lines
arch/x86/kernel/cpu/spec_ctrl.c line 10
Quoted 6 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +6
-0+#include <linux/uaccess.h> + +#include <asm/spec_ctrl.h> +#include <asm/cpufeature.h> + +unsigned int dynamic_ibrs__read_mostly ;WTH is
dynamic_ibrs?
Dynamic ibrs because we enable the IBRS in MSR 0x48 entering the kernel and disable it when we exit the kernel.
Quoted 24 lines
arch/x86/kernel/cpu/spec_ctrl.c line 22
Quoted 13 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +12
-0+EXPORT_SYMBOL_GPL (dynamic_ibrs); + +enum { +IBRS_DISABLED , +/* in host kernel, disabled in guest and userland */ +IBRS_ENABLED , +/* in host kernel and host userland, disabled in guest */ +IBRS_ENABLED_USER , +IBRS_MAX =IBRS_ENABLED_USER , +}; +static unsigned int ibrs_enabled; +static bool ibrs_admin_disabled;Srsly?!
That's *three* variables controlling IBRS. This needs simplification.
arch/x86/kernel/cpu/spec_ctrl.c line 25
Quoted 3 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +3
-0+ +/* mutex to serialize IBRS control changes */ +DEFINE_MUTEX (spec_ctrl_mutex);static
Quoted 5 lines
arch/x86/kernel/cpu/spec_ctrl.c line 29
Quoted 3 lines
arch/x86/kernel/cpu/spec_ctrl.c new file +3
-0+void scan_spec_ctrl_feature (struct cpuinfo_x86 *c) +{ +if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) {What is !c->cpu_index? Checking whether this is the BSP? What for?
This is to ensure that we only do the operation once during boot.
Thanks.
Tim
PATCH v2 5/8 x86/idle: Disable IBRS entering idle and enable it on wakeup
In reply to Tim ChenClear IBRS on idle entry and set it on idle exit into kernel on mwait. When we are in mwait, we are not running but if we leave IBRS on, it will affect the performance on the sibling hardware thread. So we disable IBRS and reenable it when we wake up.
--- arch/x86/include/asm/mwait.h | 13 +++++++++++++ arch/x86/include/asm/spec_ctrl.h | 35 +++++++++++++++++++++++++++++++++++ arch/x86/kernel/process.c | 9 +++++++-- 3 files changed, 55 insertions(+), 2 deletions(-)
arch/x86/include/asm/mwait.h +13 -0
@@ -6,6 +6,7 @@#include <linux/sched/idle.h> #include <asm/cpufeature.h> +#include <asm/spec_ctrl.h> #define MWAIT_SUBSTATE_MASK 0xf#define MWAIT_CSTATE_MASK 0xf
@@ -106,9 +107,21 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)mb (); } +/* +* CPUs run faster with speculation protection +* disabled. All CPU threads in a core must +* disable speculation protection for it to be +* disabled. Disable it while we are idle so the +* other hyperthread can run fast. +* +* Interrupts have been disabled at this point. +*/ + +unprotected_speculation_begin ();__monitor ((void *)¤t_thread_info ()->flags,0 ,0 );if (!need_resched ())__mwait (eax, ecx); +unprotected_speculation_end (); }current_clr_polling (); }
arch/x86/include/asm/spec_ctrl.h +35 -0
@@ -12,4 +12,39 @@ bool ibrs_inuse(void);extern unsigned int dynamic_ibrs; +static inline void __disable_indirect_speculation (void ) +{ +native_wrmsrl (MSR_IA32_SPEC_CTRL ,SPEC_CTRL_FEATURE_ENABLE_IBRS ); +} + +static inline void __enable_indirect_speculation (void ) +{ +native_wrmsrl (MSR_IA32_SPEC_CTRL ,SPEC_CTRL_FEATURE_DISABLE_IBRS ); +} + +/* +* Interrupts must be disabled to begin unprotected speculation. +* Otherwise interrupts could come in and start running in unprotected mode. +*/ + +static inline void unprotected_speculation_begin (void ) +{ +lockdep_assert_irqs_disabled (); +if (dynamic_ibrs) +__enable_indirect_speculation (); +} + +static inline void unprotected_speculation_end (void ) +{ +if (dynamic_ibrs) { +__disable_indirect_speculation (); + }else { +/* +* rmb prevent unwanted speculation when we +* are setting IBRS +*/ +rmb (); + } +} +#endif /* _ASM_X86_SPEC_CTRL_H */
arch/x86/kernel/process.c +7 -2
@@ -39,6 +39,7 @@#include <asm/switch_to.h> #include <asm/desc.h> #include <asm/prctl.h> +#include <asm/spec_ctrl.h> /* * per-CPU TSS segments. Threads are completely 'soft' on Linux,
@@ -461,11 +462,15 @@ static __cpuidle void mwait_idle(void)mb ();/* quirk */ } +unprotected_speculation_begin ();__monitor ((void *)¤t_thread_info ()->flags,0 ,0 ); -if (!need_resched ()) +if (!need_resched ()) {__sti_mwait (0 ,0 ); -else +unprotected_speculation_end (); + }else { +unprotected_speculation_end ();local_irq_enable (); + }trace_cpu_idle_rcuidle (PWR_EVENT_EXIT ,smp_processor_id ()); }else {local_irq_enable ();
Signature · 1 line
2.9.4
PATCH v2 6/8 x86/microcode: Recheck IBRS features on microcode reload
In reply to Tim ChenOn new microcode write, check whether IBRS
is present by rescanning IBRS feature.
--- arch/x86/include/asm/spec_ctrl.h | 1 + arch/x86/kernel/cpu/microcode/core.c | 4 ++++ arch/x86/kernel/cpu/spec_ctrl.c | 18 ++++++++++-------- 3 files changed, 15 insertions(+), 8 deletions(-)
arch/x86/include/asm/spec_ctrl.h +1 -0
@@ -8,6 +8,7 @@#include <asm/microcode.h> void scan_spec_ctrl_feature (struct cpuinfo_x86 *c); +void rescan_spec_ctrl_feature (struct cpuinfo_x86 *c);bool ibrs_inuse (void );extern unsigned int dynamic_ibrs;
arch/x86/kernel/cpu/microcode/core.c +4 -0
@@ -40,6 +40,7 @@#include <asm/processor.h> #include <asm/cmdline.h> #include <asm/setup.h> +#include <asm/spec_ctrl.h> #define DRIVER_VERSION "2.2"
@@ -444,6 +445,9 @@ static ssize_t microcode_write(struct file *file, const char __user *buf,if (ret >0 )perf_check_microcode (); +/* check spec_ctrl capabilities */ +rescan_spec_ctrl_feature (&boot_cpu_data); +mutex_unlock (µcode_mutex);put_online_cpus ();
arch/x86/kernel/cpu/spec_ctrl.c +10 -8
@@ -24,13 +24,18 @@ static bool ibrs_admin_disabled;/* mutex to serialize IBRS control changes */ DEFINE_MUTEX (spec_ctrl_mutex); +static inline void set_ibrs_feature (void ) +{ +if (!ibrs_admin_disabled) { + dynamic_ibrs =1 ; + ibrs_enabled =IBRS_ENABLED ; + } +} +void scan_spec_ctrl_feature (struct cpuinfo_x86 *c) {if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) { -if (!ibrs_admin_disabled) { - dynamic_ibrs =1 ; - ibrs_enabled =IBRS_ENABLED ; - } +set_ibrs_feature (); } }EXPORT_SYMBOL_GPL (scan_spec_ctrl_feature);
@@ -43,10 +48,7 @@ void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c) {mutex_lock (&spec_ctrl_mutex);if (boot_cpu_has (X86_FEATURE_SPEC_CTRL )) { -if (!ibrs_admin_disabled) { - dynamic_ibrs =1 ; - ibrs_enabled =IBRS_ENABLED ; - } +set_ibrs_feature (); }mutex_unlock (&spec_ctrl_mutex); }
Signature · 1 line
2.9.4
Re: [PATCH v2 6/8] x86/microcode: Recheck IBRS features on microcode reload
Quoted 4 lines · Tim Chen
On new microcode write, check whether IBRS
is present by rescanning IBRS feature.Signed-off-by Tim Chen <[email protected]>
Didn't we also give you a patch which fixed up the error handling on the microcode reload, when it fails on only one CPU?
Borislav Petkov
maintainerRe: [PATCH v2 6/8] x86/microcode: Recheck IBRS features on microcode reload
In reply to Tim ChenQuoted 35 lines · Tim Chen
On new microcode write, check whether IBRS
is present by rescanning IBRS feature.Signed-off-by Tim Chen <[email protected]>--- arch/x86/include/asm/spec_ctrl.h | 1 + arch/x86/kernel/cpu/microcode/core.c | 4 ++++ arch/x86/kernel/cpu/spec_ctrl.c | 18 ++++++++++-------- 3 files changed, 15 insertions(+), 8 deletions(-)arch/x86/include/asm/spec_ctrl.h +1
-0@@ -8,6 +8,7 @@#include <asm/microcode.h> void scan_spec_ctrl_feature (struct cpuinfo_x86 *c); +void rescan_spec_ctrl_feature (struct cpuinfo_x86 *c);bool ibrs_inuse (void );extern unsigned int dynamic_ibrs;arch/x86/kernel/cpu/microcode/core.c +1
-0@@ -40,6 +40,7 @@#include <asm/processor.h> #include <asm/cmdline.h> #include <asm/setup.h> +#include <asm/spec_ctrl.h> #define DRIVER_VERSION "2.2"
arch/x86/kernel/cpu/microcode/core.c static ssize_t microcode_write(struct file *file, const char __user *buf, · line 449
Quoted 5 lines
arch/x86/kernel/cpu/microcode/core.c +2
-0@@ -444,6 +445,9 @@ static ssize_t microcode_write(struct file *file, const char __user *buf,if (ret >0 )perf_check_microcode (); +/* check spec_ctrl capabilities */ +rescan_spec_ctrl_feature (&boot_cpu_data);
Needs to be under the if like perf_check_microcode().
Quoted 32 lines
arch/x86/kernel/cpu/microcode/core.c +1
-0+mutex_unlock (µcode_mutex);put_online_cpus ();arch/x86/kernel/cpu/spec_ctrl.c +9
-4@@ -24,13 +24,18 @@ static bool ibrs_admin_disabled;/* mutex to serialize IBRS control changes */ DEFINE_MUTEX (spec_ctrl_mutex); +static inline void set_ibrs_feature (void ) +{ +if (!ibrs_admin_disabled) { + dynamic_ibrs =1 ; + ibrs_enabled =IBRS_ENABLED ; + } +} +void scan_spec_ctrl_feature (struct cpuinfo_x86 *c) {if ((!c->cpu_index) && (boot_cpu_has (X86_FEATURE_SPEC_CTRL ))) { -if (!ibrs_admin_disabled) { - dynamic_ibrs =1 ; - ibrs_enabled =IBRS_ENABLED ; - } +set_ibrs_feature (); } }EXPORT_SYMBOL_GPL (scan_spec_ctrl_feature);
arch/x86/kernel/cpu/spec_ctrl.c void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c) · line 51
Quoted 8 lines
arch/x86/kernel/cpu/spec_ctrl.c +1
-4@@ -43,10 +48,7 @@ void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c) {mutex_lock (&spec_ctrl_mutex);if (boot_cpu_has (X86_FEATURE_SPEC_CTRL )) { -if (!ibrs_admin_disabled) { - dynamic_ibrs =1 ; - ibrs_enabled =IBRS_ENABLED ; - } +set_ibrs_feature ();
As already stated, this does not work. You want to check the CPUID leaf after the microcode reload and then set X86_FEATURE_SPEC_CTRL on each CPU.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.PATCH v2 7/8 x86: Do not use dynamic IBRS if retpoline is enabled
In reply to Tim ChenDon't use dynamic IBRS that toggle IBRS on entry/exit to kernel code if retpoline is used.
--- arch/x86/kernel/cpu/spec_ctrl.c | 2 ++ 1 file changed, 2 insertions(+)
arch/x86/kernel/cpu/spec_ctrl.c +2 -0
@@ -26,10 +26,12 @@ DEFINE_MUTEX(spec_ctrl_mutex);static inline void set_ibrs_feature (void ) { +#if !defined(RETPOLINE )if (!ibrs_admin_disabled) { dynamic_ibrs =1 ; ibrs_enabled =IBRS_ENABLED ; } +#endif }void scan_spec_ctrl_feature (struct cpuinfo_x86 *c)
Signature · 1 line
2.9.4
PATCH v2 8/8 x86: Use IBRS for firmware update path
In reply to Tim ChenFrom: David Woodhouse <[email protected]>
We are impervious to the indirect branch prediction attack with retpoline but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.
--- arch/x86/include/asm/apm.h | 6 ++++++ arch/x86/include/asm/efi.h | 17 +++++++++++++-- arch/x86/include/asm/spec_ctrl.h | 3 +++ arch/x86/kernel/cpu/spec_ctrl.c | 45 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 2 deletions(-)
arch/x86/include/asm/apm.h +6 -0
@@ -7,6 +7,8 @@#ifndef _ASM_X86_MACH_DEFAULT_APM_H#define _ASM_X86_MACH_DEFAULT_APM_H +#include <asm/spec_ctrl.h> +#ifdef APM_ZERO_SEGS # define APM_DO_ZERO_SEGS \ "pushl %%ds\n\t" \
@@ -28,6 +30,7 @@ static inline void apm_bios_call_asm(u32 func, u32 ebx_in, u32 ecx_in, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx, u32 *esi) { +unprotected_firmware_begin ();/* * N.B. We do NOT need a cld after the BIOS call * because we always save and restore the flags.
@@ -44,6 +47,7 @@ static inline void apm_bios_call_asm(u32 func, u32 ebx_in, u32 ecx_in, "=S" (*esi) : "a" (func), "b" (ebx_in), "c" (ecx_in) : "memory", "cc"); +unprotected_firmware_end (); } static inline bool apm_bios_call_simple_asm(u32 func, u32 ebx_in,
@@ -52,6 +56,7 @@ static inline bool apm_bios_call_simple_asm(u32 func, u32 ebx_in,int cx, dx, si;bool error; +unprotected_firmware_begin ();/* * N.B. We do NOT need a cld after the BIOS call * because we always save and restore the flags.
@@ -68,6 +73,7 @@ static inline bool apm_bios_call_simple_asm(u32 func, u32 ebx_in, "=S" (si) : "a" (func), "b" (ebx_in), "c" (ecx_in) : "memory", "cc"); +unprotected_firmware_end ();return error; }
arch/x86/include/asm/efi.h +15 -2
@@ -6,6 +6,7 @@#include <asm/pgtable.h> #include <asm/processor-flags.h> #include <asm/tlb.h> +#include <asm/spec_ctrl.h> /* * We map the EFI regions needed for runtime services non-contiguously,
@@ -36,8 +37,18 @@extern asmlinkage unsigned long efi_call_phys (void *, ...); -#define arch_efi_call_virt_setup () kernel_fpu_begin() -#define arch_efi_call_virt_teardown () kernel_fpu_end() +#define arch_efi_call_virt_setup () \ +{( \ + kernel_fpu_begin(); \ + unprotected_firmware_begin(); \ +)} + +#define arch_efi_call_virt_teardown () \ +{( \ + unprotected_firmware_end(); \ + kernel_fpu_end(); \ +)} +/* * Wrap all the virtual calls in a way that forces the parameters on the stack.
@@ -73,6 +84,7 @@ struct efi_scratch {efi_sync_low_kernel_mappings (); \preempt_disable (); \__kernel_fpu_begin (); \ +unprotected_firmware_begin (); \ \if (efi_scratch.use_pgd) { \ efi_scratch.prev_cr3 =__read_cr3 (); \
@@ -91,6 +103,7 @@ struct efi_scratch {__flush_tlb_all (); \ } \ \ +unprotected_firmware_end (); \__kernel_fpu_end (); \preempt_enable (); \ })
arch/x86/include/asm/spec_ctrl.h +3 -0
@@ -11,6 +11,9 @@ void scan_spec_ctrl_feature(struct cpuinfo_x86 *c);void rescan_spec_ctrl_feature (struct cpuinfo_x86 *c);bool ibrs_inuse (void ); +void unprotected_firmware_begin (void ); +void unprotected_firmware_end (void ); +extern unsigned int dynamic_ibrs;static inline void __disable_indirect_speculation (void )
arch/x86/kernel/cpu/spec_ctrl.c +45 -0
60 lines not on this page · raw message
Signature · 1 line
2.9.4
Re: [PATCH v2 8/8] x86: Use IBRS for firmware update path
Quoted 6 lines · Tim Chen
From: David Woodhouse <[email protected]>
We are impervious to the indirect branch prediction attack with retpoline but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.
Quoted 8 lines
Signed-off-by David Woodhouse <[email protected]>Signed-off-by Tim Chen <[email protected]>--- arch/x86/include/asm/apm.h | 6 ++++++ arch/x86/include/asm/efi.h | 17 +++++++++++++-- arch/x86/include/asm/spec_ctrl.h | 3 +++ arch/x86/kernel/cpu/spec_ctrl.c | 45 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 2 deletions(-)
The files that Alan pointed out as being missed in the last version of this patch don't seem to be updated here either :(
thanks,
greg k-h
Re: [PATCH v2 8/8] x86: Use IBRS for firmware update path
In reply to Tim Chenarch/x86/kernel/cpu/spec_ctrl.c static int __init debugfs_spec_ctrl(void) · line 189
Quoted 13 lines · Tim Chen
arch/x86/kernel/cpu/spec_ctrl.c +13
-0+void unprotected_firmware_begin (void ) +{ +if (firmware_ibrs) { +__disable_indirect_speculation (); + }else { +/* +* rmb prevent unwanted speculation when we +* are setting IBRS +*/ +rmb (); + } +} +EXPORT_SYMBOL_GPL (unprotected_firmware_begin);
What modules need these functions?
And again, if they are needed, we need a better naming scheme for them. Prefix them all with "firmware_" perhaps to make it a bit nicer?
thanks,
greg k-h
Quoted 6 lines · Tim Chen
Thanks to everyone for the feedback on the initial posting. This is an updated patchset and I hope I've captured all the review comments. I've done a lot of code clean up per everyone's comments. Please let me know if I've missed something.
Should also have mentioned that the patch series applies on top of 4.15-rc6.
Tim
Quoted 11 lines · Tim Chen
Quoted 7 lines · Tim Chen
Thanks to everyone for the feedback on the initial posting. This is an updated patchset and I hope I've captured all the review comments. I've done a lot of code clean up per everyone's comments. Please let me know if I've missed something.
Should also have mentioned that the patch series applies on top of 4.15-rc6.
If you're going to set/clear the X86_FEATURE_RETPOLINE and X86_FEATURE_RETPOLINE_AMD features (sorry, there are two of them now. Blame Linus), you probably want to base on the retpoline branch instead? It was Arjan's choice to put retpoline first, wasn't it?
In the last few days we've gone from a coherent patch series which was far from perfect but which was at least basically complete and functional, to disparate patch series all of which conflict. And some of which haven't even been posted publicly yet at all (IBPB, and I still haven't actually seen the variant 1 patches in public).
It would be really good to pull everything together, even if it's going to receive significant bikeshedding.
Quoted 9 lines · Tim Chen
Tim Chen (8):
x86/feature: Detect the x86 IBRS feature to control Speculation x86/enter: MACROS to set/clear IBRS
x86/enter: Use IBRS on syscall and interrupts x86/spec_ctrl: Add sysctl knobs to enable/disableSPEC_CTRLfeature x86/idle: Disable IBRS entering idle and enable it on wakeup x86/microcode: Recheck IBRS features on microcode reload
x86: Do not use dynamic IBRS if retpoline is enabled
x86: Use IBRS for firmware update path
.. where's the KVM bit? We need to set IBRS on vmexit too.