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

[Josh Poimboeuf <jpoimboe@redhat.com>]

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