Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Wed, 2018-02-14 at 10:36 -0600, Tom Lendacky wrote:
> On 2/14/2018 10:11 AM, David Woodhouse wrote:
> > 
> > 
> > 
> > On Wed, 2018-02-14 at 10:07 -0600, Tom Lendacky wrote:
> > > 
> > > Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
> > > also defines the STIBP bit for this MSR, and if that bit had been set by
> > > BIOS for example, these writes will clear it.  And who knows what future
> > > bits may be defined and how they'll be used.
> >
> > We don't use STIBP. If one day we do decide to set it in userspace for
>
> Right, I understand the kernel doesn't use STIBP, that's why I mentioned
> BIOS as an example.

BIOS has no business setting this for us either. Either we support it
and turn it on in the kernel ourselves, or it's off.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Wed, 2018-02-14 at 10:36 -0600, Tom Lendacky wrote:
> On 2/14/2018 10:11 AM, David Woodhouse wrote:
> > 
> > 
> > 
> > On Wed, 2018-02-14 at 10:07 -0600, Tom Lendacky wrote:
> > > 
> > > Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
> > > also defines the STIBP bit for this MSR, and if that bit had been set by
> > > BIOS for example, these writes will clear it.  And who knows what future
> > > bits may be defined and how they'll be used.
> >
> > We don't use STIBP. If one day we do decide to set it in userspace for
>
> Right, I understand the kernel doesn't use STIBP, that's why I mentioned
> BIOS as an example.

BIOS has no business setting this for us either. Either we support it
and turn it on in the kernel ourselves, or it's off.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[Tom Lendacky]

On 2/14/2018 10:11 AM, David Woodhouse wrote:
> 
> 
> On Wed, 2018-02-14 at 10:07 -0600, Tom Lendacky wrote:
>> Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
>> also defines the STIBP bit for this MSR, and if that bit had been set by
>> BIOS for example, these writes will clear it.  And who knows what future
>> bits may be defined and how they'll be used.
> 
> We don't use STIBP. If one day we do decide to set it in userspace for

Right, I understand the kernel doesn't use STIBP, that's why I mentioned
BIOS as an example.

> "sensitive" processes, if we're done having the debate about what those
> are, then that seems unlikely to conflict what what this code is doing
> anyway, as we would presumably *clear* it again on the way back into
> the kernel.
> 
> I certainly don't want to add a read/modify/write cycle here just to

Right, definitely to be avoided.  Maybe the value could be tracked in a
per-cpu variable so you never have to read it before the write.  Just
change the bit in question and write.  Not sure that's really feasible
though.

> cope with some hypothetical future use case for STIBP, when there would
> be better ways to cope.

Just putting it out there, no worries.

Thanks,
Tom

> 

Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[Tom Lendacky]

On 2/14/2018 10:11 AM, David Woodhouse wrote:
> 
> 
> On Wed, 2018-02-14 at 10:07 -0600, Tom Lendacky wrote:
>> Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
>> also defines the STIBP bit for this MSR, and if that bit had been set by
>> BIOS for example, these writes will clear it.  And who knows what future
>> bits may be defined and how they'll be used.
> 
> We don't use STIBP. If one day we do decide to set it in userspace for

Right, I understand the kernel doesn't use STIBP, that's why I mentioned
BIOS as an example.

> "sensitive" processes, if we're done having the debate about what those
> are, then that seems unlikely to conflict what what this code is doing
> anyway, as we would presumably *clear* it again on the way back into
> the kernel.
> 
> I certainly don't want to add a read/modify/write cycle here just to

Right, definitely to be avoided.  Maybe the value could be tracked in a
per-cpu variable so you never have to read it before the write.  Just
change the bit in question and write.  Not sure that's really feasible
though.

> cope with some hypothetical future use case for STIBP, when there would
> be better ways to cope.

Just putting it out there, no worries.

Thanks,
Tom

> 

Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Wed, 2018-02-14 at 10:07 -0600, Tom Lendacky wrote:
> Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
> also defines the STIBP bit for this MSR, and if that bit had been set by
> BIOS for example, these writes will clear it.  And who knows what future
> bits may be defined and how they'll be used.

We don't use STIBP. If one day we do decide to set it in userspace for
"sensitive" processes, if we're done having the debate about what those
are, then that seems unlikely to conflict what what this code is doing
anyway, as we would presumably *clear* it again on the way back into
the kernel.

I certainly don't want to add a read/modify/write cycle here just to
cope with some hypothetical future use case for STIBP, when there would
be better ways to cope.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[David Woodhouse]

On Wed, 2018-02-14 at 10:07 -0600, Tom Lendacky wrote:
> Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
> also defines the STIBP bit for this MSR, and if that bit had been set by
> BIOS for example, these writes will clear it.  And who knows what future
> bits may be defined and how they'll be used.

We don't use STIBP. If one day we do decide to set it in userspace for
"sensitive" processes, if we're done having the debate about what those
are, then that seems unlikely to conflict what what this code is doing
anyway, as we would presumably *clear* it again on the way back into
the kernel.

I certainly don't want to add a read/modify/write cycle here just to
cope with some hypothetical future use case for STIBP, when there would
be better ways to cope.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[Tom Lendacky]

On 2/14/2018 7:44 AM, David Woodhouse wrote:
> Retpoline means the kernel is safe because it has no indirect branches.
> But firmware isn't, so use IBRS for firmware calls if it's available.
> 
> Block preemption while IBRS is set, although in practice the call sites
> already had to be doing that.
> 
> Ignore hpwdt.c for now. It's taking spinlocks and calling into firmware
> code, from an NMI handler. I don't want to touch that with a bargepole.
> 
> Signed-off-by: David Woodhouse 
> ---
>  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/nospec-branch.h | 39 
> +++-
>  arch/x86/kernel/cpu/bugs.c   | 12 ++-
>  5 files changed, 63 insertions(+), 12 deletions(-)
> 

...  ...

> +/*
> + * With retpoline, we must use IBRS to restrict branch prediction
> + * before calling into firmware.
> + */
> +static inline void firmware_restrict_branch_speculation_start(void)
> +{
> + preempt_disable();
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> +   X86_FEATURE_USE_IBRS_FW);
> +}
> +
> +static inline void firmware_restrict_branch_speculation_end(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);
> + preempt_enable();
>  }

Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
also defines the STIBP bit for this MSR, and if that bit had been set by
BIOS for example, these writes will clear it.  And who knows what future
bits may be defined and how they'll be used.

Thanks,
Tom

>  
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index d71c8b5..bfca937 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -300,6 +300,15 @@ static void __init spectre_v2_select_mitigation(void)
>   setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
>   pr_info("Spectre v2 mitigation: Enabling Indirect Branch 
> Prediction Barrier\n");
>   }
> +
> + /*
> +  * Retpoline means the kernel is safe because it has no indirect
> +  * branches. But firmware isn't, so use IBRS to protect that.
> +  */
> + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> + pr_info("Enabling Restricted Speculation for firmware calls\n");
> + }
>  }
>  
>  #undef pr_fmt
> @@ -326,8 +335,9 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct 
> device_attribute *attr, c
>   if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
>   return sprintf(buf, "Not affected\n");
>  
> - return sprintf(buf, "%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> + return sprintf(buf, "%s%s%s%s\n", 
> spectre_v2_strings[spectre_v2_enabled],
>  boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
> +boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
>  spectre_v2_module_string());
>  }
>  #endif
> 

Re: [PATCH 1/4] x86/speculation: Use IBRS if available before calling into firmware

[Tom Lendacky]

On 2/14/2018 7:44 AM, David Woodhouse wrote:
> Retpoline means the kernel is safe because it has no indirect branches.
> But firmware isn't, so use IBRS for firmware calls if it's available.
> 
> Block preemption while IBRS is set, although in practice the call sites
> already had to be doing that.
> 
> Ignore hpwdt.c for now. It's taking spinlocks and calling into firmware
> code, from an NMI handler. I don't want to touch that with a bargepole.
> 
> Signed-off-by: David Woodhouse 
> ---
>  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/nospec-branch.h | 39 
> +++-
>  arch/x86/kernel/cpu/bugs.c   | 12 ++-
>  5 files changed, 63 insertions(+), 12 deletions(-)
> 

...  ...

> +/*
> + * With retpoline, we must use IBRS to restrict branch prediction
> + * before calling into firmware.
> + */
> +static inline void firmware_restrict_branch_speculation_start(void)
> +{
> + preempt_disable();
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,
> +   X86_FEATURE_USE_IBRS_FW);
> +}
> +
> +static inline void firmware_restrict_branch_speculation_end(void)
> +{
> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
> +   X86_FEATURE_USE_IBRS_FW);
> + preempt_enable();
>  }

Shouldn't these writes to the MSR be just for the IBRS bit?  The spec
also defines the STIBP bit for this MSR, and if that bit had been set by
BIOS for example, these writes will clear it.  And who knows what future
bits may be defined and how they'll be used.

Thanks,
Tom

>  
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index d71c8b5..bfca937 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -300,6 +300,15 @@ static void __init spectre_v2_select_mitigation(void)
>   setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
>   pr_info("Spectre v2 mitigation: Enabling Indirect Branch 
> Prediction Barrier\n");
>   }
> +
> + /*
> +  * Retpoline means the kernel is safe because it has no indirect
> +  * branches. But firmware isn't, so use IBRS to protect that.
> +  */
> + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> + pr_info("Enabling Restricted Speculation for firmware calls\n");
> + }
>  }
>  
>  #undef pr_fmt
> @@ -326,8 +335,9 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct 
> device_attribute *attr, c
>   if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
>   return sprintf(buf, "Not affected\n");
>  
> - return sprintf(buf, "%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> + return sprintf(buf, "%s%s%s%s\n", 
> spectre_v2_strings[spectre_v2_enabled],
>  boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
> +boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
>  spectre_v2_module_string());
>  }
>  #endif
>