[MODERATED] [PATCH v6] boot-time control

[MODERATED] [PATCH v6] boot-time control

[Jiri Kosina]

Introduce 'l1tf=' boot option to allow for boot-time switching of 
mitigation that is used on CPUs affected by L1TF.

The possible values are

	full
		Provide all available mitigations for L1TF
		vulnerability (disable HT, perform PTE bit
		inversion, allow hypervisors to know that
		they should provide all mitigations).
	full,force
		Equal to 'full', plus provides a guarantee that
		HT stays disabled during the whole lifetime of
		the system (see 'nosmt=force', which is what is
		implied by this option).
	novirt
		Provide all available mitigations needed
		for running on bare metal (PTE bit inversion),
		while not applying mitigations needed for
		VM isolation. Hypervisors will isse a
		warning when the first VM is being started in
		a pontentially insecure configuraion.
	novirt,nowarn
		Claim "I don't care at all about this issue".
		The PTE bit inversion (bare metal mitigation)
		will still be performed, but hypervisors will
		not issue a warning when a VM is being started
		in a potentially insecure configuration.

Default is 'novirt'.

Let KVM adhere to these semantics, which means:

	- 'lt1f=full,force'	: start performing L1D flushes
	- 'l1tf=full'		: start performing L1D flushes,
				  warn on VM start if SMT has
				  been runtime enabled
	- 'l1tf=novirt'		: warn on first VM start
	- 'l1tf=novirt,nowarn'	: no extra action is taken

This makes the KVM's private 'nosmt' option redundant, and
as it is a bit non-systematic anyway (this is something to
control globally, not on hypervisor level), let's just remove
that option.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

v5->v6:
	- 'full' implies 'nosmt', 'full,force' implies nosmt=force;
	  print KVM warnings accordingly (one state more, and having
	  bitflags would be needed for clarity)
	- now that we have full and full,force, drop KVM's private
	  nosmt option
	- drop compile-time option to chose the default default :)
	- typo/grammar fixes

v4->v5:
	- rebase on top of KVM bundle

v3->v4:
        - unconfuse the meaning of 'off', both in the documentation and in 
          the code (spotted by Josh)

v2->v3:
        - provide l1tf=[full,novirt,off]
        - provide config option to chose the default
        - let KVM warn in novirt case

v1->v2: add forgotten dependency on X86_BUG_L1TF


 Documentation/admin-guide/kernel-parameters.txt | 34 ++++++++++++---
 arch/x86/include/asm/processor.h                |  8 ++++
 arch/x86/kernel/cpu/bugs.c                      | 55 +++++++++++++++++++++++--
 arch/x86/kvm/vmx.c                              | 39 +++++++++++++-----
 include/linux/cpu.h                             |  2 +
 kernel/cpu.c                                    |  9 +++-
 6 files changed, 126 insertions(+), 21 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 4f790566ad91..ae62716f8481 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1946,12 +1946,6 @@
 			[KVM,ARM] Allow use of GICv4 for direct injection of
 			LPIs.
 
-	kvm-intel.nosmt=[KVM,Intel] If the L1TF CPU bug is present (CVE-2018-3620)
-			and the system has SMT (aka Hyper-Threading) enabled then
-			don't allow guests to be created.
-
-			Default is 0 (allow guests to be created).
-
 	kvm-intel.ept=	[KVM,Intel] Disable extended page tables
 			(virtualized MMU) support on capable Intel chips.
 			Default is 1 (enabled)
@@ -1989,6 +1983,34 @@
 			feature (tagged TLBs) on capable Intel chips.
 			Default is 1 (enabled)
 
+	l1tf=           [X86] Control mitigation of the L1TF vulnerability on
+			      affected CPUs
+			full
+				Provide all available mitigations for L1TF
+				vulnerability (disable HT, perform PTE bit
+				inversion, allow hypervisors to know that
+				they should provide all mitigations).
+			full,force
+				Equal to 'full', plus provides a guarantee that
+				HT stays disabled during the whole lifetime of
+				the system (see 'nosmt=force', which is what is
+				implied by this option).
+			novirt
+				Provide all available mitigations needed
+				for running on bare metal (PTE bit inversion),
+				while not applying mitigations needed for
+				VM isolation. Hypervisors will isse a
+				warning when the first VM is being started in
+				a pontentially insecure configuraion.
+			novirt,nowarn
+				Claim "I don't care at all about this issue".
+				The PTE bit inversion (bare metal mitigation)
+				will still be performed, but hypervisors will
+				not issue a warning when a VM is being started
+				in a potentially insecure configuration.
+
+			Default is 'novirt'.
+
 	l2cr=		[PPC]
 
 	l3cr=		[PPC]
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 7e3ac5eedcd6..3be9dab9f27f 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -982,4 +982,12 @@ bool xen_set_default_idle(void);
 void stop_this_cpu(void *dummy);
 void df_debug(struct pt_regs *regs, long error_code);
 void microcode_check(void);
+
+enum l1tf_mitigations {
+	L1TF_MITIGATION_NOVIRT_NOWARN,
+	L1TF_MITIGATION_NOVIRT,
+	L1TF_MITIGATION_FULL,
+	L1TF_MITIGATION_FULL_FORCE
+};
+enum l1tf_mitigations get_l1tf_mitigation(void);
 #endif /* _ASM_X86_PROCESSOR_H */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 50500cea6eba..62a160a75101 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -657,6 +657,16 @@ void x86_spec_ctrl_setup_ap(void)
 
 #undef pr_fmt
 #define pr_fmt(fmt)	"L1TF: " fmt
+
+/* Default mitigation for L1TF-affected CPUs */
+static enum l1tf_mitigations l1tf_mitigation = L1TF_MITIGATION_NOVIRT;
+
+enum l1tf_mitigations get_l1tf_mitigation(void)
+{
+	return l1tf_mitigation;
+}
+EXPORT_SYMBOL(get_l1tf_mitigation);
+
 static void __init l1tf_select_mitigation(void)
 {
 	u64 half_pa;
@@ -664,6 +674,18 @@ static void __init l1tf_select_mitigation(void)
 	if (!boot_cpu_has_bug(X86_BUG_L1TF))
 		return;
 
+	switch (get_l1tf_mitigation()) {
+	case L1TF_MITIGATION_FULL_FORCE:
+		cpu_smt_disable(true);
+		break;
+	case L1TF_MITIGATION_FULL:
+		cpu_smt_disable(false);
+		break;
+	case L1TF_MITIGATION_NOVIRT:
+	case L1TF_MITIGATION_NOVIRT_NOWARN:
+		break;
+	}
+
 #if CONFIG_PGTABLE_LEVELS == 2
 	pr_warn("Kernel not compiled for PAE. No mitigation for L1TF\n");
 	return;
@@ -682,10 +704,39 @@ static void __init l1tf_select_mitigation(void)
 
 	setup_force_cpu_cap(X86_FEATURE_L1TF_PTEINV);
 }
+
+static int __init l1tf_cmdline(char *str)
+{
+	if (!boot_cpu_has_bug(X86_BUG_L1TF))
+		return 0;
+
+	if (!str)
+		return 0;
+
+	if (!strcmp(str, "full"))
+		l1tf_mitigation = L1TF_MITIGATION_FULL;
+	else if (!strcmp(str, "full,force"))
+		l1tf_mitigation = L1TF_MITIGATION_FULL_FORCE;
+	else if (!strcmp(str, "novirt"))
+		l1tf_mitigation = L1TF_MITIGATION_NOVIRT;
+	else if (!strcmp(str, "novirt,nowarn"))
+		l1tf_mitigation = L1TF_MITIGATION_NOVIRT_NOWARN;
+
+	return 0;
+}
+early_param("l1tf", l1tf_cmdline);
+
 #undef pr_fmt
 
 #ifdef CONFIG_SYSFS
 
+static const char *l1tf_states[] = {
+	[L1TF_MITIGATION_FULL]		= "Mitigation: Full",
+	[L1TF_MITIGATION_FULL_FORCE]	= "Mitigation: Full [force]",
+	[L1TF_MITIGATION_NOVIRT]	= "Mitigation: Page Table Inversion [novirt]",
+	[L1TF_MITIGATION_NOVIRT_NOWARN]	= "Mitigation: Page Table Inversion [novirt,nowarn]"
+};
+
 static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
 			       char *buf, unsigned int bug)
 {
@@ -712,9 +763,7 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
 		return sprintf(buf, "%s\n", ssb_strings[ssb_mode]);
 
 	case X86_BUG_L1TF:
-		if (boot_cpu_has(X86_FEATURE_L1TF_PTEINV))
-			return sprintf(buf, "Mitigation: Page Table Inversion\n");
-		break;
+		return sprintf(buf, "%s\n", l1tf_states[get_l1tf_mitigation()]);
 
 	default:
 		break;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index eb7c207a3bc3..a69a74d3c9b0 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -71,9 +71,6 @@ static const struct x86_cpu_id vmx_cpu_id[] = {
 };
 MODULE_DEVICE_TABLE(x86cpu, vmx_cpu_id);
 
-static bool __read_mostly nosmt;
-module_param(nosmt, bool, S_IRUGO);
-
 static bool __read_mostly enable_vpid = 1;
 module_param_named(vpid, enable_vpid, bool, 0444);
 
@@ -10539,19 +10536,39 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 	return ERR_PTR(err);
 }
 
-#define L1TF_MSG "SMT enabled with L1TF CPU bug present. Refer to CVE-2018-3620 for details.\n"
+#define L1TF_MSG_NOVIRT "L1TF CPU bug present and virtualization mitigation disabled, data leak possible. Refer to CVE-2018-3620 for details.\n"
+#define L1TF_MSG_SMT "L1TF CPU bug present and SMT enabled, data leak possible. Refer to CVE-2018-3620 for details.\n"
 
 static int vmx_vm_init(struct kvm *kvm)
 {
 	if (!ple_gap)
 		kvm->arch.pause_in_guest = true;
 
-	if (boot_cpu_has(X86_BUG_L1TF) && cpu_smt_control == CPU_SMT_ENABLED) {
-		if (nosmt) {
-			pr_err(L1TF_MSG);
-			return -EOPNOTSUPP;
+	if (boot_cpu_has(X86_BUG_L1TF)) {
+		switch (get_l1tf_mitigation()) {
+			case L1TF_MITIGATION_NOVIRT_NOWARN:
+				/* The 'I explicitly don't care' flag is set */
+				break;
+			case L1TF_MITIGATION_NOVIRT:
+				/*
+				 * Warn upon starting the first VM in a
+				 * potentially insecure environment.
+				 */
+				pr_warn_once(L1TF_MSG_NOVIRT);
+				break;
+			case L1TF_MITIGATION_FULL:
+				/*
+				 * Warn if SMT has been runtime-enabled. We can't
+				 * really warn only once here, as SMT state is
+				 * not constant.
+				 */
+				if (cpu_smt_control == CPU_SMT_ENABLED)
+					pr_warn(L1TF_MSG_SMT);
+				break;
+			case L1TF_MITIGATION_FULL_FORCE:
+				/* All is good, proceed without hiccup */
+				break;
 		}
-		pr_warn(L1TF_MSG);
 	}
 	return 0;
 }
@@ -13237,7 +13254,9 @@ static int __init vmx_setup_l1d_flush(void)
 
 	if (vmentry_l1d_flush == VMENTER_L1D_FLUSH_NEVER ||
 	    !boot_cpu_has_bug(X86_BUG_L1TF) ||
-	    vmx_l1d_use_msr_save_list())
+	    vmx_l1d_use_msr_save_list() ||
+	    get_l1tf_mitigation() == L1TF_MITIGATION_NOVIRT ||
+	    get_l1tf_mitigation() == L1TF_MITIGATION_NOVIRT_NOWARN)
 		return 0;
 
 	if (!boot_cpu_has(X86_FEATURE_FLUSH_L1D)) {
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 7532cbf27b1d..3a3b5c4b1d4a 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -177,8 +177,10 @@ enum cpuhp_smt_control {
 
 #if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
 extern enum cpuhp_smt_control cpu_smt_control;
+void cpu_smt_disable(bool force);
 #else
 # define cpu_smt_control		(CPU_SMT_ENABLED)
+static inline void cpu_smt_disable(bool force) { }
 #endif
 
 #endif /* _LINUX_CPU_H_ */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 5a00ebdf98c6..5c9ff956e8eb 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -347,13 +347,18 @@ EXPORT_SYMBOL_GPL(cpu_hotplug_enable);
 enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
 EXPORT_SYMBOL_GPL(cpu_smt_control);
 
-static int __init smt_cmdline_disable(char *str)
+void __init cpu_smt_disable(bool force)
 {
 	cpu_smt_control = CPU_SMT_DISABLED;
-	if (str && !strcmp(str, "force")) {
+	if (force) {
 		pr_info("SMT: Force disabled\n");
 		cpu_smt_control = CPU_SMT_FORCE_DISABLED;
 	}
+}
+
+static int __init smt_cmdline_disable(char *str)
+{
+	cpu_smt_disable(str && !strcmp(str, "force"));
 	return 0;
 }
 early_param("nosmt", smt_cmdline_disable);

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [PATCH v6] boot-time control

[Andi Kleen]

> -#define L1TF_MSG "SMT enabled with L1TF CPU bug present. Refer to CVE-2018-3620 for details.\n"
> +#define L1TF_MSG_NOVIRT "L1TF CPU bug present and virtualization mitigation disabled, data leak possible. Refer to CVE-2018-3620 for details.\n"
> +#define L1TF_MSG_SMT "L1TF CPU bug present and SMT enabled, data leak possible. Refer to CVE-2018-3620 for details.\n"

This should refer to the Document I posted yesterday (which should be
also merged into the patchkit)

Also probably should not refer the CVE, it is usually not that useful.

-Andi

[MODERATED] Re: [PATCH v6] boot-time control

[Jiri Kosina]

On Fri, 6 Jul 2018, speck for Andi Kleen wrote:

> This should refer to the Document I posted yesterday (which should be
> also merged into the patchkit)
> 
> Also probably should not refer the CVE, it is usually not that useful.

I don't know ... I'd bet the number of sysadmins who are able to look up a 
CVE is probably much higher than number of sysadmins capable/willing to 
wander through kernel sources to find information.

What would be your suggested wording so that it's digestible by a sysadmin 
who's never seen a kernel source tree before? There are tons of those 
guys.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v6] boot-time control

[Thomas Gleixner]

On Fri, 6 Jul 2018, speck for Jiri Kosina wrote:
> On Fri, 6 Jul 2018, speck for Andi Kleen wrote:
> 
> > This should refer to the Document I posted yesterday (which should be
> > also merged into the patchkit)
> > 
> > Also probably should not refer the CVE, it is usually not that useful.
> 
> I don't know ... I'd bet the number of sysadmins who are able to look up a 
> CVE is probably much higher than number of sysadmins capable/willing to 
> wander through kernel sources to find information.
> 
> What would be your suggested wording so that it's digestible by a sysadmin 
> who's never seen a kernel source tree before? There are tons of those 
> guys.

There is that and that document needs some love before it's going to be
applied.

The only way to refer to it is when it's written in RST format so it
becomes part of the kernel documentation and then we'd need a stable URL
for it as well.

But having it in RST so it's part of the HTML/PDF documentation would be a
good thing in any case.

Thanks,

	tglx

[MODERATED] Re: [PATCH v6] boot-time control

[Luck, Tony]

On Fri, Jul 06, 2018 at 09:23:57PM +0200, speck for Jiri Kosina wrote:
> I don't know ... I'd bet the number of sysadmins who are able to look up a 
> CVE is probably much higher than number of sysadmins capable/willing to 
> wander through kernel sources to find information.

If you feel that you must have the CVE here, then shouldn't you have
CVE-2018-3646 which covers the virtualization aspects of L1TF rather
than CVE-2018-3620 which just covers OS and SMM implications?

-Tony

[MODERATED] Re: [PATCH v6] boot-time control

[Jiri Kosina]

On Fri, 6 Jul 2018, speck for Josh Poimboeuf wrote:

> Will you be doing the sysfs control next?

Once this is in, I am planning to do so, but this alone has bigger 
priority on my list, so I want to have that in first.

[ ... snip the stylistic fixes / typos ... ]
> > +			case L1TF_MITIGATION_FULL:
> > +				/*
> > +				 * Warn if SMT has been runtime-enabled. We can't
> > +				 * really warn only once here, as SMT state is
> > +				 * not constant.
> > +				 */
> > +				if (cpu_smt_control == CPU_SMT_ENABLED)
> > +					pr_warn(L1TF_MSG_SMT);
> 
> I think I said this sounded ok on IRC, but...
> 
> This repeated warning will be very annoying for those people enabling 
> SMT on purpose.  I think a one-time warning would be better.

No problem with that.

On Fri, 6 Jul 2018, speck for Luck, Tony wrote:

> If you feel that you must have the CVE here, 

I don't; if Intel has any better suggestions that would be equally 
alerting the sysadmin, let's change it to whatever works the best.

> then shouldn't you have CVE-2018-3646 which covers the virtualization 
> aspects of L1TF rather than CVE-2018-3620 which just covers OS and SMM 
> implications?

Good catch, I copied the CVE number from what we already have in speck.git 
(via 26acfb666a473) and didn't cross-check it. So either it has to be 
changed via this (modified(*)) patch, or it should be fixed independetly 
in speck.git.

(*) Thomas, all the comments so far are rather cosmetic ... if nothing 
    else pops up, do you prefer to fix that up on your side when applying, 
    or should I send v8?

Thanks,

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [PATCH v6] boot-time control

[Josh Poimboeuf]

On Fri, Jul 06, 2018 at 10:19:39PM +0200, speck for Jiri Kosina wrote:
> On Fri, 6 Jul 2018, speck for Josh Poimboeuf wrote:
> 
> > Will you be doing the sysfs control next?
> 
> Once this is in, I am planning to do so, but this alone has bigger 
> priority on my list, so I want to have that in first.
> 
> [ ... snip the stylistic fixes / typos ... ]
> > > +			case L1TF_MITIGATION_FULL:
> > > +				/*
> > > +				 * Warn if SMT has been runtime-enabled. We can't
> > > +				 * really warn only once here, as SMT state is
> > > +				 * not constant.
> > > +				 */
> > > +				if (cpu_smt_control == CPU_SMT_ENABLED)
> > > +					pr_warn(L1TF_MSG_SMT);
> > 
> > I think I said this sounded ok on IRC, but...
> > 
> > This repeated warning will be very annoying for those people enabling 
> > SMT on purpose.  I think a one-time warning would be better.
> 
> No problem with that.
> 
> On Fri, 6 Jul 2018, speck for Luck, Tony wrote:
> 
> > If you feel that you must have the CVE here, 
> 
> I don't; if Intel has any better suggestions that would be equally 
> alerting the sysadmin, let's change it to whatever works the best.
> 
> > then shouldn't you have CVE-2018-3646 which covers the virtualization 
> > aspects of L1TF rather than CVE-2018-3620 which just covers OS and SMM 
> > implications?
> 
> Good catch, I copied the CVE number from what we already have in speck.git 
> (via 26acfb666a473) and didn't cross-check it. So either it has to be 
> changed via this (modified(*)) patch, or it should be fixed independetly 
> in speck.git.
> 
> (*) Thomas, all the comments so far are rather cosmetic ... if nothing 
>     else pops up, do you prefer to fix that up on your side when applying, 
>     or should I send v8?

With the above changed to a one-time warning (with comment adjusted
accordingly) and typos fixed:

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[MODERATED] Re: [PATCH v6] boot-time control

[Josh Poimboeuf]

On Fri, Jul 06, 2018 at 08:47:25PM +0200, speck for Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH v6] x86/bugs, kvm: introduce boot-time control of L1TF mitigations
> 
> Introduce 'l1tf=' boot option to allow for boot-time switching of 
> mitigation that is used on CPUs affected by L1TF.

This looks great to me.  A few minor comments below.  Will you be doing
the sysfs control next?

> 
> The possible values are
> 
> 	full
> 		Provide all available mitigations for L1TF
> 		vulnerability (disable HT, perform PTE bit
> 		inversion, allow hypervisors to know that
> 		they should provide all mitigations).
> 	full,force
> 		Equal to 'full', plus provides a guarantee that
> 		HT stays disabled during the whole lifetime of
> 		the system (see 'nosmt=force', which is what is
> 		implied by this option).
> 	novirt
> 		Provide all available mitigations needed
> 		for running on bare metal (PTE bit inversion),
> 		while not applying mitigations needed for
> 		VM isolation. Hypervisors will isse a

"issue"

> 		warning when the first VM is being started in
> 		a pontentially insecure configuraion.

"potentially"

> @@ -1989,6 +1983,34 @@
>  			feature (tagged TLBs) on capable Intel chips.
>  			Default is 1 (enabled)
>  
> +	l1tf=           [X86] Control mitigation of the L1TF vulnerability on
> +			      affected CPUs

colon

> +			full
> +				Provide all available mitigations for L1TF

for *the* L1TF

> +				vulnerability (disable HT, perform PTE bit
> +				inversion, allow hypervisors to know that
> +				they should provide all mitigations).
> +			full,force
> +				Equal to 'full', plus provides a guarantee that
> +				HT stays disabled during the whole lifetime of
> +				the system (see 'nosmt=force', which is what is
> +				implied by this option).
> +			novirt
> +				Provide all available mitigations needed
> +				for running on bare metal (PTE bit inversion),
> +				while not applying mitigations needed for
> +				VM isolation. Hypervisors will isse a

"issue"

> @@ -10539,19 +10536,39 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  	return ERR_PTR(err);
>  }
>  
> -#define L1TF_MSG "SMT enabled with L1TF CPU bug present. Refer to CVE-2018-3620 for details.\n"
> +#define L1TF_MSG_NOVIRT "L1TF CPU bug present and virtualization mitigation disabled, data leak possible. Refer to CVE-2018-3620 for details.\n"
> +#define L1TF_MSG_SMT "L1TF CPU bug present and SMT enabled, data leak possible. Refer to CVE-2018-3620 for details.\n"
>  
>  static int vmx_vm_init(struct kvm *kvm)
>  {
>  	if (!ple_gap)
>  		kvm->arch.pause_in_guest = true;
>  
> -	if (boot_cpu_has(X86_BUG_L1TF) && cpu_smt_control == CPU_SMT_ENABLED) {
> -		if (nosmt) {
> -			pr_err(L1TF_MSG);
> -			return -EOPNOTSUPP;
> +	if (boot_cpu_has(X86_BUG_L1TF)) {
> +		switch (get_l1tf_mitigation()) {
> +			case L1TF_MITIGATION_NOVIRT_NOWARN:
> +				/* The 'I explicitly don't care' flag is set */
> +				break;
> +			case L1TF_MITIGATION_NOVIRT:
> +				/*
> +				 * Warn upon starting the first VM in a
> +				 * potentially insecure environment.
> +				 */
> +				pr_warn_once(L1TF_MSG_NOVIRT);
> +				break;
> +			case L1TF_MITIGATION_FULL:
> +				/*
> +				 * Warn if SMT has been runtime-enabled. We can't
> +				 * really warn only once here, as SMT state is
> +				 * not constant.
> +				 */
> +				if (cpu_smt_control == CPU_SMT_ENABLED)
> +					pr_warn(L1TF_MSG_SMT);

I think I said this sounded ok on IRC, but...

This repeated warning will be very annoying for those people enabling
SMT on purpose.  I think a one-time warning would be better.

-- 
Josh

Re: [PATCH v6] boot-time control

[Thomas Gleixner]

On Fri, 6 Jul 2018, speck for Jiri Kosina wrote:
> +/* Default mitigation for L1TF-affected CPUs */
> +static enum l1tf_mitigations l1tf_mitigation = L1TF_MITIGATION_NOVIRT;

__ro_after_init

> +enum l1tf_mitigations get_l1tf_mitigation(void)
> +{
> +	return l1tf_mitigation;
> +}
> +EXPORT_SYMBOL(get_l1tf_mitigation);

We can just export the variable

> +static const char *l1tf_states[] = {
> +	[L1TF_MITIGATION_FULL]		= "Mitigation: Full",
> +	[L1TF_MITIGATION_FULL_FORCE]	= "Mitigation: Full [force]",
> +	[L1TF_MITIGATION_NOVIRT]	= "Mitigation: Page Table Inversion [novirt]",
> +	[L1TF_MITIGATION_NOVIRT_NOWARN]	= "Mitigation: Page Table Inversion [novirt,nowarn]"
> +};
> +
>  static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
>  			       char *buf, unsigned int bug)
>  {
> @@ -712,9 +763,7 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
>  		return sprintf(buf, "%s\n", ssb_strings[ssb_mode]);
>  
>  	case X86_BUG_L1TF:
> -		if (boot_cpu_has(X86_FEATURE_L1TF_PTEINV))
> -			return sprintf(buf, "Mitigation: Page Table Inversion\n");
> -		break;
> +		return sprintf(buf, "%s\n", l1tf_states[get_l1tf_mitigation()]);

Hmm. This is not telling the actual VMX protection state, which might be
different than what the command line paraameter or default selected.

>  static int vmx_vm_init(struct kvm *kvm)
>  {
>  	if (!ple_gap)
>  		kvm->arch.pause_in_guest = true;
>  
> -	if (boot_cpu_has(X86_BUG_L1TF) && cpu_smt_control == CPU_SMT_ENABLED) {
> -		if (nosmt) {
> -			pr_err(L1TF_MSG);
> -			return -EOPNOTSUPP;
> +	if (boot_cpu_has(X86_BUG_L1TF)) {
> +		switch (get_l1tf_mitigation()) {
> +			case L1TF_MITIGATION_NOVIRT_NOWARN:
> +				/* The 'I explicitly don't care' flag is set */
> +				break;
> +			case L1TF_MITIGATION_NOVIRT:
> +				/*
> +				 * Warn upon starting the first VM in a
> +				 * potentially insecure environment.
> +				 */
> +				pr_warn_once(L1TF_MSG_NOVIRT);

This should only warn when l1d flush is disabled and SMT enabled.

> @@ -13237,7 +13254,9 @@ static int __init vmx_setup_l1d_flush(void)
>  
>  	if (vmentry_l1d_flush == VMENTER_L1D_FLUSH_NEVER ||
>  	    !boot_cpu_has_bug(X86_BUG_L1TF) ||
> -	    vmx_l1d_use_msr_save_list())
> +	    vmx_l1d_use_msr_save_list() ||
> +	    get_l1tf_mitigation() == L1TF_MITIGATION_NOVIRT ||
> +	    get_l1tf_mitigation() == L1TF_MITIGATION_NOVIRT_NOWARN)

I think that's wrong. If the module parameter selected a l1d flush method
then overriding it from the l1tf parameter makes no sense.

I'd rather force the parameter to flush when L1TF_MITIGATION_FULL* is
selected.

Thanks,

	tglx

[MODERATED] Re: [PATCH v6] boot-time control

[Jiri Kosina]

On Sun, 8 Jul 2018, speck for Thomas Gleixner wrote:

> > +static const char *l1tf_states[] = {
> > +	[L1TF_MITIGATION_FULL]		= "Mitigation: Full",
> > +	[L1TF_MITIGATION_FULL_FORCE]	= "Mitigation: Full [force]",
> > +	[L1TF_MITIGATION_NOVIRT]	= "Mitigation: Page Table Inversion [novirt]",
> > +	[L1TF_MITIGATION_NOVIRT_NOWARN]	= "Mitigation: Page Table Inversion [novirt,nowarn]"
> > +};
> > +
> >  static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
> >  			       char *buf, unsigned int bug)
> >  {
> > @@ -712,9 +763,7 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
> >  		return sprintf(buf, "%s\n", ssb_strings[ssb_mode]);
> >  
> >  	case X86_BUG_L1TF:
> > -		if (boot_cpu_has(X86_FEATURE_L1TF_PTEINV))
> > -			return sprintf(buf, "Mitigation: Page Table Inversion\n");
> > -		break;
> > +		return sprintf(buf, "%s\n", l1tf_states[get_l1tf_mitigation()]);
> 
> Hmm. This is not telling the actual VMX protection state, which might be
> different than what the command line paraameter or default selected.

So we can either display only the current state of affairs, such as

	Mitigation: Page Table Inversion, VMX L1D=[confined,always,off], SMT=[on,off]

or somehow try to display both what has been selected on cmdline, and what 
is the current state.

If noone has objections, I think I'll just go with displaying the current 
state (as above), ignoring the cmdline parameter altogether.

I'll wait a bit if there is any more feedback apart from your and Josh's 
and send v7 probably tomorrow.

Thanks,

-- 
Jiri Kosina
SUSE Labs