[permalink] [raw]

Subject: Re: [PATCH 7/7] x86/microcode: Recheck IBRS features on microcode reload

On Thu, Jan 04, 2018 at 07:28:58PM +0100, Borislav Petkov wrote:
> On Thu, Jan 04, 2018 at 09:56:48AM -0800, Tim Chen wrote:
> > On new microcode write, check whether IBRS
> > is present by rescanning scattered CPU features.
> >
> > Signed-off-by: Tim Chen <[email protected]>
> > ---
> > arch/x86/kernel/cpu/microcode/core.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> > index c4fa4a8..44b9355 100644
> > --- a/arch/x86/kernel/cpu/microcode/core.c
> > +++ b/arch/x86/kernel/cpu/microcode/core.c
> > @@ -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,11 @@ static ssize_t microcode_write(struct file *file, const char __user *buf,
> > if (ret > 0)
> > perf_check_microcode();
> >
> > + /* check spec_ctrl capabilities */
> > + mutex_lock(&spec_ctrl_mutex);
> > + init_scattered_cpuid_features(&boot_cpu_data);
>
> No need for that - make a specific function like perf_check_microcode()
> which checks only the IBRS bit and updates stuff accordingly.

It would be better I agree. I've got this:

void spec_ctrl_rescan_cpuid(void)
{
int cpu;

if (use_ibp_disable)
return;
mutex_lock(&spec_ctrl_mutex);
if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
/* detect spec ctrl related cpuid additions */
init_scattered_cpuid_features(&boot_cpu_data);
spec_ctrl_init(&boot_cpu_data);

/*
* The SPEC_CTRL and IBPB_SUPPORT cpuid bits may have
* just been set in the boot_cpu_data, transfer them
* to the per-cpu data too. This must run after
* spec_ctrl_init() to take care of
* setup_force_cpu_cap() too.
*/
if (cpu_has_spec_ctrl())
for_each_online_cpu(cpu)
set_cpu_cap(&cpu_data(cpu),
X86_FEATURE_SPEC_CTRL);
if (boot_cpu_has(X86_FEATURE_IBPB_SUPPORT))
for_each_online_cpu(cpu)
set_cpu_cap(&cpu_data(cpu),
X86_FEATURE_IBPB_SUPPORT);
}
mutex_unlock(&spec_ctrl_mutex);
}

However we've to start somewhere so that is a simpler start..

2018-01-04 18:34:40

by Andrea Arcangeli

[permalink] [raw]

On Thu, 2018-01-04 at 11:00 -0800, Linus Torvalds wrote:
> On Thu, Jan 4, 2018 at 9:56 AM, Tim Chen <[email protected]>
> wrote:
> >
> >
> > Speculation on Skylake and later requires these patches ("dynamic
> > IBRS")
> > be used instead of retpoline[1].
> Can somebody explain this part?
>
> I was assuming that retpoline would work around this issue on all
> uarchs.
>
> This seems to say "retpoline does nothing on Skylake+"

On Skylake the target for a 'ret' instruction may also come from the
BTB. So if you ever let the RSB (which remembers where the 'call's came
from get empty, you end up vulnerable.

Other than the obvious call stack of more than 16 calls in depth,
there's also a big list of other things which can empty the RSB,
including an SMI.

Which basically makes retpoline on Skylake+ *very* hard to use
reliably. The plan is to use IBRS there and not retpoline.

Attachments:

smime.p7s (5.09 kB)

2018-01-04 19:33:37

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH 0/7] IBRS patch series

On Thu, Jan 4, 2018 at 11:19 AM, David Woodhouse <[email protected]> wrote:
>
> On Skylake the target for a 'ret' instruction may also come from the
> BTB. So if you ever let the RSB (which remembers where the 'call's came
> from get empty, you end up vulnerable.

That sounds like it could cause mispredicts, but it doesn't sound _exploitable_.

Sure, interrupts in between the call instruction and the 'ret' could
overflow the return stack. And we could migrate to another CPU. And so
apparently SMM clears the return stack too.

... but again, none of them sound even remotely _exploitable_.

Remember: it's not mispredicts that leak information. It's *exploits"
that use forced very specific mispredicts to leak information.

There's a big difference there. And I think patch authors should keep
that difference in mind.

For example, flushing the BTB at kernel entry doesn't mean that later
in-kernel indirect branches don't get predicted, and doesn't even mean
that they don't get mis-predicted. It only means that an exploit can't
pre-populate those things and use them for exploits.

Linus

2018-01-04 19:39:29

On Thu, 2018-01-04 at 19:40 +0000, Andrew Cooper wrote:
>
> Also remember that sibling threads share a BTB, so you can't rely on
> isolated straight-line codepath on the current cpu for safety. (e.g. by
> issuing an IBPB on every entry to supervisor mode).

That is just one of a whole litany of reasons the RSB gets flushed,
*many* of which were individually sufficient for us to throw our toys
out of the pram and say "OK, this doesn't work on Skylake".

Attachments:

smime.p7s (5.09 kB)

2018-01-04 19:58:08

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 1/7] x86/feature: Detect the x86 feature to control Speculation

On Thu, Jan 04, 2018 at 09:56:42AM -0800, Tim Chen wrote:
> cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature
> IA32_SPEC_CTRL (0x48) and IA32_PRED_CMD (0x49)
> IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS)
> IA32_PRED_CMD, bit0 – Indirect Branch Prediction Barrier (IBPB)
>
> 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.
>
> Setting of IBPB ensures that earlier code's behavior does not control later
> indirect branch predictions. It is used when context switching to new
> untrusted address space. Unlike IBRS, it is a command MSR and does not retain
> its state.
>
> * 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
>
> * IBRS is not guaranteed to differentiate two applications that use
> the same CR3 due to recycling. Software can use an IBPB command when
> recycling a page table base address.
>
> * VMM software can similarly use an IBPB when recycling a controlling
> VMCS pointer address
>
> 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 | 7 +++++++
> arch/x86/kernel/cpu/scattered.c | 1 +
> 3 files changed, 9 insertions(+)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 86c68cb..431f393 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -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 */

You should have gotten a build warning with just this patch, please also
update tools/arch/x86/include/asm/cpufeatures.h to fix that.

And why not use a free slot, (7*32+13) or (7*32+12) is free, right?

Or were you just trying to make backports "easier"? :)

thanks,

greg k-h

2018-01-04 20:00:33

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 3/7] x86/enter: Use IBRS on syscall and interrupts

On Thu, Jan 04, 2018 at 09:56:44AM -0800, Tim Chen wrote:
>
> That is a minor inefficiency only, but we can eliminate it by saving
> the MSR when entering the NMI in save_paranoid and restoring it when
> exiting the NMI.

Any hints as to what exactly "minor" means in cycles here? :)

thanks,

greg k-h

2018-01-04 20:05:12

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 5/7] x86: Use IBRS for firmware update path

On Thu, Jan 04, 2018 at 09:56:46AM -0800, Tim Chen wrote:
> 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.

Wait, what?

Maybe it's just the wine from dinner talking, but if the firmware has
issues, we have bigger things to worry about here, right? It already
handed over the "chain of trust" to us, so we have already implicitly
trusted that the firmware was correct here. So why do we need to do
anything about firmware calls in this manner?

Or am I totally missing something else here?

thanks,

greg k-h

2018-01-04 20:10:35

On Thu, 2018-01-04 at 09:56 -0800, Tim Chen wrote:
> @@ -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_formware_end();

Just an obvious typo (form/firm) but I thought I'd report it.
> }
>
> 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_formware_end();
> return error;
> }

Same here.
--
Yves-Alexis

Attachments:

signature.asc (488.00 B)
This is a digitally signed message part

2018-01-04 20:58:43

by Tim Chen

[permalink] [raw]

Subject: Re: [PATCH 6/7] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

On 01/04/2018 12:16 PM, Greg KH wrote:
> On Thu, Jan 04, 2018 at 09:56:47AM -0800, Tim Chen wrote:
>> 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/ibrs_enabled will turn off IBRS
>> echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel
>> echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel
>>
>> The implementation was updated with input from Andrea Arcangeli.
>>
>> Signed-off-by: Tim Chen <[email protected]>
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 4 +
>> arch/x86/include/asm/spec_ctrl.h | 163 +++++++++++++++++++-----
>> arch/x86/kernel/cpu/Makefile | 1 +
>> arch/x86/kernel/cpu/scattered.c | 10 ++
>> arch/x86/kernel/cpu/spec_ctrl.c | 124 ++++++++++++++++++
>> 5 files changed, 270 insertions(+), 32 deletions(-)
>> create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 5dfd262..d64f49f 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -2573,6 +2573,10 @@
>> noexec=on: enable non-executable mappings (default)
>> noexec=off: disable non-executable mappings
>>
>> + noibrs [X86]
>> + Don't use indirect branch restricted speculation (IBRS)
>> + feature.
>> +
>> nosmap [X86]
>> Disable SMAP (Supervisor Mode Access Prevention)
>> even if it is supported by processor.
>> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
>> index 23b2804..2c35571 100644
>> --- a/arch/x86/include/asm/spec_ctrl.h
>> +++ b/arch/x86/include/asm/spec_ctrl.h
>> @@ -1,13 +1,17 @@
>> #ifndef _ASM_X86_SPEC_CTRL_H
>> #define _ASM_X86_SPEC_CTRL_H
>>
>> -#include <linux/stringify.h>
>> #include <asm/msr-index.h>
>> #include <asm/cpufeatures.h>
>> -#include <asm/alternative-asm.h>
>> +
>> +#define SPEC_CTRL_IBRS_INUSE (1<<0) /* OS enables IBRS usage */
>> +#define SPEC_CTRL_IBRS_SUPPORTED (1<<1) /* System supports IBRS */
>> +#define SPEC_CTRL_IBRS_ADMIN_DISABLED (1<<2) /* Admin disables IBRS */
>>
>> #ifdef __ASSEMBLY__
>>
>> +.extern spec_ctrl_ibrs
>> +
>> .macro PUSH_MSR_REGS
>> pushq %rax
>> pushq %rcx
>> @@ -27,35 +31,63 @@
>> .endm
>>
>> .macro ENABLE_IBRS
>> - ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> + testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> + jz .Lskip_\@
>> +
>> PUSH_MSR_REGS
>> WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>> POP_MSR_REGS
>> -10:
>> +
>> + jmp .Ldone_\@
>> +.Lskip_\@:
>> + /*
>> + * prevent speculation beyond here as we could want to
>> + * stop speculation by enabling IBRS
>> + */
>> + lfence
>> +.Ldone_\@:
>> .endm
>>
>> .macro DISABLE_IBRS
>> - ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> + testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> + jz .Lskip_\@
>> +
>> PUSH_MSR_REGS
>> WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
>> POP_MSR_REGS
>> -10:
>> +
>> +.Lskip_\@:
>> .endm
>>
>> .macro ENABLE_IBRS_CLOBBER
>> - ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> + testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> + jz .Lskip_\@
>> +
>> WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>> -10:
>> +
>> + jmp .Ldone_\@
>> +.Lskip_\@:
>> + /*
>> + * prevent speculation beyond here as we could want to
>> + * stop speculation by enabling IBRS
>> + */
>> + lfence
>> +.Ldone_\@:
>> .endm
>>
>> .macro DISABLE_IBRS_CLOBBER
>> - ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> + testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> + jz .Lskip_\@
>> +
>> WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
>> -10:
>> +
>> +.Lskip_\@:
>> .endm
>>
>> .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
>> - ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> + testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
>> + jz .Lskip_\@
>> +
>> movl $MSR_IA32_SPEC_CTRL, %ecx
>> rdmsr
>> movl %eax, \save_reg
>> @@ -63,22 +95,103 @@
>> movl $0, %edx
>> movl $SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
>> wrmsr
>> -10:
>> +
>> + jmp .Ldone_\@
>> +.Lskip_\@:
>> + /*
>> + * prevent speculation beyond here as we could want to
>> + * stop speculation by enabling IBRS
>> + */
>> + lfence
>> +.Ldone_\@:
>> .endm
>>
>> .macro RESTORE_IBRS_CLOBBER save_reg:req
>> - ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
>> + testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_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
>> -10:
>> +
>> + jmp .Ldone_\@
>> +.Lskip_\@:
>> + /*
>> + * prevent speculation beyond here as we could want to
>> + * stop speculation by enabling IBRS
>> + */
>> + lfence
>> +.Ldone_\@:
>> .endm
>>
>> #else
>> #include <asm/microcode.h>
>>
>> +extern int spec_ctrl_ibrs;
>> +extern struct mutex spec_ctrl_mutex;
>> +extern unsigned int ibrs_enabled;
>> +
>> +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,
>
> Yup, not a bit field, your documentation is wrong :(
>
>> +};
>> +
>> +
>> +static inline void set_ibrs_inuse(void)
>> +{
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> + spec_ctrl_ibrs |= SPEC_CTRL_IBRS_INUSE;
>> +}
>> +
>> +static inline void clear_ibrs_inuse(void)
>> +{
>> + spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_INUSE;
>> +}
>> +
>> +static inline int ibrs_inuse(void)
>> +{
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_INUSE)
>> + return 1;
>> + else
>> + /*
>> + * prevent speculation beyond here as we could want to
>> + * stop speculation by enabling IBRS with this check
>> + */
>> + rmb();
>> + return 0;
>> +}
>> +
>> +static inline void set_ibrs_supported(void)
>> +{
>> + spec_ctrl_ibrs |= SPEC_CTRL_IBRS_SUPPORTED;
>> + if (!(spec_ctrl_ibrs & SPEC_CTRL_IBRS_ADMIN_DISABLED))
>> + set_ibrs_inuse();
>> + else
>> + /*
>> + * prevent speculation beyond here as we could want to
>> + * stop speculation by enabling IBRS
>> + */
>> + rmb();
>> +}
>> +
>> +static inline void set_ibrs_disabled(void)
>> +{
>> + spec_ctrl_ibrs |= SPEC_CTRL_IBRS_ADMIN_DISABLED;
>> + if (ibrs_inuse())
>> + clear_ibrs_inuse();
>> +}
>> +
>> +static inline void clear_ibrs_disabled(void)
>> +{
>> + spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_ADMIN_DISABLED;
>> +}
>> +
>> static inline void __disable_indirect_speculation(void)
>> {
>> native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
>> @@ -96,21 +209,14 @@ static inline void __enable_indirect_speculation(void)
>> static inline void unprotected_speculation_begin(void)
>> {
>> WARN_ON_ONCE(!irqs_disabled());
>> - if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> + if (ibrs_inuse())
>> __enable_indirect_speculation();
>> }
>>
>> static inline void unprotected_speculation_end(void)
>> {
>> - if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> + if (ibrs_inuse())
>> __disable_indirect_speculation();
>> - else
>> - /*
>> - * If we intended to disable indirect speculation
>> - * but come here due to mis-speculation, we need
>> - * to stop the mis-speculation with rmb.
>> - */
>> - rmb();
>> }
>>
>>
>> @@ -121,20 +227,13 @@ static inline void unprotected_speculation_end(void)
>> */
>> static inline void unprotected_firmware_begin(void)
>> {
>> - if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> + if (ibrs_inuse())
>> __disable_indirect_speculation();
>> - else
>> - /*
>> - * If we intended to disable indirect speculation
>> - * but come here due to mis-speculation, we need
>> - * to stop the mis-speculation with rmb.
>> - */
>> - rmb();
>> }
>>
>> static inline void unprotected_firmware_end(void)
>> {
>> - if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> + if (ibrs_inuse())
>> __enable_indirect_speculation();
>> }
>>
>> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
>> index 90cb82d..a25f1ab 100644
>> --- a/arch/x86/kernel/cpu/Makefile
>> +++ b/arch/x86/kernel/cpu/Makefile
>> @@ -24,6 +24,7 @@ obj-y += match.o
>> obj-y += bugs.o
>> obj-$(CONFIG_CPU_FREQ) += 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
>> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
>> index bc50c40..2d23a2fe 100644
>> --- a/arch/x86/kernel/cpu/scattered.c
>> +++ b/arch/x86/kernel/cpu/scattered.c
>> @@ -8,6 +8,7 @@
>> #include <asm/processor.h>
>>
>> #include <asm/apic.h>
>> +#include <asm/spec_ctrl.h>
>>
>> struct cpuid_bit {
>> u16 feature;
>> @@ -56,6 +57,15 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>>
>> if (regs[cb->reg] & (1 << cb->bit))
>> set_cpu_cap(c, cb->feature);
>> +
>> + }
>> + if (!c->cpu_index) {
>> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
>> + printk(KERN_INFO "FEATURE SPEC_CTRL Present\n");
>> + set_ibrs_supported();
>> + if (ibrs_inuse())
>> + ibrs_enabled = IBRS_ENABLED;
>> + }
>> }
>> }
>>
>> diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
>> new file mode 100644
>> index 0000000..6946678
>> --- /dev/null
>> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
>> @@ -0,0 +1,124 @@
>> +#include <linux/export.h>
>
> No copyright or SPDX line?
>
> At least it doesn't have the old horrid Intel header boilerplate, so I
> should be thankful of that. But it isn't ok like this either, sorry.
>
>> +#include <linux/init.h>
>> +#include <linux/mutex.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include <asm/spec_ctrl.h>
>> +#include <asm/cpufeature.h>
>> +
>> +/*
>> + * spec_ctrl_ibrs
>> + * bit 0 = indicate if ibrs is currently in use
>> + * bit 1 = indicate if system supports ibrs
>> + * bit 2 = indicate if admin disables ibrs
>
> Why bits and not integer values? Can you mix them?

spec_ctrl_ibrs has bit fields for internal speculation
control and is not exposed to user.
ibrs_enabled is reported to the user and takes the enum
values

+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,
+};
+

>
>> + */
>> +
>> +int spec_ctrl_ibrs;
>> +EXPORT_SYMBOL(spec_ctrl_ibrs);
>
> Why is this exported? What module will ever need this?
>
> And horrid global symbol name, it doesn't say what it is in an obvious
> way just by looking at it.
>
>> +
>> +/* mutex to serialize IBRS control changes */
>> +DEFINE_MUTEX(spec_ctrl_mutex);
>> +EXPORT_SYMBOL(spec_ctrl_mutex);
>
> Why export this? You only ever use it in one place? Again, what module
> needs it?

It is needed for places that potentially have more than one cpu
touching spec_ctrl_ibrs values. I'll see if I can consolidate
them to spec_ctrl.c to avoid the export.

>
> And I have to ask, EXPORT_SYMBOL_GPL() for this and spec_ctrl_ibrs as
> well?
>
>> +unsigned int ibrs_enabled __read_mostly;
>> +EXPORT_SYMBOL(ibrs_enabled);
>
> Again, what module needs this?
>
> And did __read_mostly really matter in performance tests?
>
>> +
>> +static int __init noibrs(char *str)
>> +{
>> + set_ibrs_disabled();
>> +
>> + return 0;
>> +}
>> +early_param("noibrs", ex);
>> +
>> +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);
>> +}
>
> It's a single variable, why such complex debugfs for that? This should
> be handled by a helper macro already, right?
>
>> +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;
>
> 3 value "flags" are horrid :(

Unfortunately, IBRS can have 3 different usage scenarios:
1. disabled all the time
2. used only in kernel and privileged code
3. enabled all the time in both user and kernel space.

>
>> +
>> + if (enable > IBRS_MAX)
>> + return -EINVAL;
>> +
>> + mutex_lock(&spec_ctrl_mutex);
>
> Always run checkpatch.pl so you don't get grumpy kernel maintainers
> telling you to run checkpatch.pl :(
>
>> +
>> + if (enable == IBRS_DISABLED) {
>> + /* disable IBRS usage */
>> + set_ibrs_disabled();
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> + spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
>> + } else if (enable == IBRS_ENABLED) {
>> + /* enable IBRS usage in kernel */
>> + clear_ibrs_disabled();
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> + set_ibrs_inuse();
>> + else
>> + /* Platform don't support IBRS */
>> + enable = IBRS_DISABLED;
>> + } else if (enable == IBRS_ENABLED_USER) {
>> + /* enable IBRS usage in both userspace and kernel */
>> + clear_ibrs_disabled();
>> + /* don't change IBRS value once we set it to always on */
>> + clear_ibrs_inuse();
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> + spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
>> + else
>> + /* Platform don't support IBRS */
>> + enable = IBRS_DISABLED;
>> + }
>> +
>> + WRITE_ONCE(ibrs_enabled, enable);
>
> It's a debugfs write callback, why do you care about WRITE_ONCE()?

Probably not necessary now. It is serialized by spec_ctrl_mutex.

>
> thanks,
>
> greg k-h
>

2018-01-04 21:02:00

On Fri, 2018-01-05 at 06:25 +0100, Florian Weimer wrote:
>
> Retpoline also looks incompatible with CET, so future Intel CPUs will
> eventually need a different approach anyway.

CPUs with CET will have the indirect branch problem fixed, so the
retpoline ALTERNATIVE will be used which is a bare 'jmp *%reg'.

That's why we switched the ABI for the retpoline from having the target
address on the stack, to passing it in in a register.

Attachments:

smime.p7s (5.09 kB)

2018-01-05 11:14:52

by David Woodhouse

[permalink] [raw]

Subject: Re: [PATCH 1/7] x86/feature: Detect the x86 feature to control Speculation

On Thu, 2018-01-04 at 09:56 -0800, Tim Chen wrote:
> cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature
> IA32_SPEC_CTRL (0x48) and IA32_PRED_CMD (0x49)
> IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS)
> IA32_PRED_CMD, bit0 – Indirect Branch Prediction Barrier (IBPB)

In a previous iteration of these patches, hadn't you already merged
support for the AMD variant? Where'd that go?

Attachments:

smime.p7s (5.09 kB)

2018-01-05 11:16:58

On Fri, 2018-01-05 at 17:05 +0100, Andrea Arcangeli wrote:
> On Fri, Jan 05, 2018 at 03:38:24PM +0000, David Woodhouse wrote:
> >
> > We had IBRS first, and especially on Broadwell and earlier, its
> > performance really is painful.
> >
> > Then came retpoline, purely as an optimisation. A very *important*
> > performance improvement, but an optimisation nonetheless.
> >
> > When looking at optimisations, it is rare for us to say "oh, well it
> > opens up only a *small* theoretical security hole, but it's faster so
> > that's OK".
> I couldn't express any better than the above, the way I look at
> repotlines.
>
> Now seeing how this thing escalates with 2-way gcc code emission with
> amd version that will not have ibrs at all to call it around the bios
> etc... and IBRS skylake is still needed as a 3-way alternative and no
> code exists to even patch it all at boot like that, and then qemu has
> to be compiled with reptoline and 2-way alternative there too, to
> achieve ibrs 2 ibpb 1, it's not looking like the way to go to me.
>
> Not in the short term at least, and for the long term "reptoline" is
> guaranteed a totally useless effort.
>
> reptoline is like highmem kmap() etc... eventually nobody will ever
> use reptoline. reptoline is more interesting for downstream in fact if
> something where at least we won't have to deal with that forever where
> there's more control on the toolchain used, and after a certain number
> of years that code expires.
>
> I can imagine we'll unfortunately have to deal with reptoline at some
> point, but starting with reptoline at least looks backwards,
> especially if it's the long term you're planning for.

You are completely ignoring pre-Skylake here.

On pre-Skylake, retpoline is perfectly sufficient and it's a *lot*
faster than the IBRS option which is almost prohibitively slow.

We didn't do it just for fun. And it's working fine; it isn't *that*
complex.

Attachments:

smime.p7s (5.09 kB)

2018-01-05 16:42:50

by Andrea Arcangeli

[permalink] [raw]

Subject: Re: [PATCH 0/7] IBRS patch series

On Fri, Jan 05, 2018 at 04:37:30PM +0000, David Woodhouse wrote:
> You are completely ignoring pre-Skylake here.
>
> On pre-Skylake, retpoline is perfectly sufficient and it's a *lot*
> faster than the IBRS option which is almost prohibitively slow.
>
> We didn't do it just for fun. And it's working fine; it isn't *that*
> complex.

How do you enable IBRS when the CPU switches to SMM?

Do you already have this 2-way code emission from gcc and patching
with a 3-way alternatives at boot between ibrs and 2 reptoline version
emitted by gcc and alternatives between ibrs and ibpb where SPEC_CTRL
is missing on some CPU but IBPB_SUPPORT is available?

Or are you talking about having done this on a non upstream Xen build
only without the 2-way code emission for gcc?

2018-01-05 16:44:21

by Van De Ven, Arjan

[permalink] [raw]

Subject: RE: [PATCH 0/7] IBRS patch series

> On Fri, Jan 05, 2018 at 04:37:30PM +0000, David Woodhouse wrote:
> > You are completely ignoring pre-Skylake here.
> >
> > On pre-Skylake, retpoline is perfectly sufficient and it's a *lot*
> > faster than the IBRS option which is almost prohibitively slow.
> >
> > We didn't do it just for fun. And it's working fine; it isn't *that*
> > complex.
>
> How do you enable IBRS when the CPU switches to SMM?

you do not need to. SMM takes care of itself.

2018-01-05 16:46:51

On 01/05/2018 03:16 AM, David Woodhouse wrote:
> On Thu, 2018-01-04 at 09:56 -0800, Tim Chen wrote:
>>
>> + mutex_lock(&spec_ctrl_mutex);
>> +
>> + if (enable == IBRS_DISABLED) {
>> + /* disable IBRS usage */
>> + set_ibrs_disabled();
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> + spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
>> + } else if (enable == IBRS_ENABLED) {
>> + /* enable IBRS usage in kernel */
>> + clear_ibrs_disabled();
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> + set_ibrs_inuse();
>> + else
>> + /* Platform don't support IBRS */
>> + enable = IBRS_DISABLED;
>> + } else if (enable == IBRS_ENABLED_USER) {
>> + /* enable IBRS usage in both userspace and kernel */
>> + clear_ibrs_disabled();
>> + /* don't change IBRS value once we set it to always on */
>> + clear_ibrs_inuse();
>> + if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
>> + spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
>> + else
>> + /* Platform don't support IBRS */
>> + enable = IBRS_DISABLED;
>> + }
>
> This doesn't take the retpoline status into account. If we have
> retpoline, we don't need IBRS in the kernel.
>

If retpoline is used, we don't enable IBRS automatically during feature detection.
But if the admin is paranoid, he still has the choice to explicitly issue
a command to enable IBRS here.

Tim