linux-kernel - Re: [PATCH v2 25/49] KVM: x86: Harden CPU capabilities processing against out-of-scope features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7c072dac426f77953158b0c804d81c664c00d1e3.camel@redhat.com>
Date: Thu, 04 Jul 2024 21:31:18 -0400
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>,  Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Hou Wenlong
 <houwenlong.hwl@...group.com>, Kechen Lu <kechenl@...dia.com>, Oliver Upton
 <oliver.upton@...ux.dev>, Binbin Wu <binbin.wu@...ux.intel.com>, Yang
 Weijiang <weijiang.yang@...el.com>, Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 25/49] KVM: x86: Harden CPU capabilities processing
 against out-of-scope features

On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> Add compile-time assertions to verify that usage of F() and friends in
> kvm_set_cpu_caps() is scoped to the correct CPUID word, e.g. to detect
> bugs where KVM passes a feature bit from word X into word y.
> 
> Add a one-off assertion in the aliased feature macro to ensure that only
> word 0x8000_0001.EDX aliased the features defined for 0x1.EDX.
> 
> To do so, convert kvm_cpu_cap_init() to a macro and have it define a
> local variable to track which CPUID word is being initialized that is
> then used to validate usage of F() (all of the inputs are compile-time
> constants and thus can be fed into BUILD_BUG_ON()).
> 
> Redefine KVM_VALIDATE_CPU_CAP_USAGE after kvm_set_cpu_caps() to be a nop
> so that F() can be used in other flows that aren't as easily hardened,
> e.g. __do_cpuid_func_emulated() and __do_cpuid_func().
> 
> Invoke KVM_VALIDATE_CPU_CAP_USAGE() in SF() and X86_64_F() to ensure the
> validation occurs, e.g. if the usage of F() is completely compiled out
> (which shouldn't happen for boot_cpu_has(), but could happen in the future,
> e.g. if KVM were to use cpu_feature_enabled()).
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/cpuid.c | 55 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a16d6e070c11..1064e4d68718 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -61,18 +61,24 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>  	return ret;
>  }
>  
> -#define F feature_bit
> +#define F(name)							\
> +({								\
> +	KVM_VALIDATE_CPU_CAP_USAGE(name);			\
> +	feature_bit(name);					\
> +})
>  
>  /* Scattered Flag - For features that are scattered by cpufeatures.h. */
>  #define SF(name)						\
>  ({								\
>  	BUILD_BUG_ON(X86_FEATURE_##name >= MAX_CPU_FEATURES);	\
> +	KVM_VALIDATE_CPU_CAP_USAGE(name);			\
>  	(boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0);	\
>  })
>  
>  /* Features that KVM supports only on 64-bit kernels. */
>  #define X86_64_F(name)						\
>  ({								\
> +	KVM_VALIDATE_CPU_CAP_USAGE(name);			\
>  	(IS_ENABLED(CONFIG_X86_64) ? F(name) : 0);		\
>  })
>  
> @@ -95,6 +101,7 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>  #define AF(name)								\
>  ({										\
>  	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
> +	BUILD_BUG_ON(kvm_cpu_cap_init_in_progress != CPUID_8000_0001_EDX);	\
>  	feature_bit(name);							\
>  })
>  
> @@ -622,22 +629,34 @@ static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid)
>  	return *__cpuid_entry_get_reg(&entry, cpuid.reg);
>  }
>  
> -static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask)
> -{
> -	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);
> +/*
> + * Assert that the feature bit being declared, e.g. via F(), is in the CPUID
> + * word that's being initialized.  Exempt 0x8000_0001.EDX usage of 0x1.EDX
> + * features, as AMD duplicated many 0x1.EDX features into 0x8000_0001.EDX.
> + */
> +#define KVM_VALIDATE_CPU_CAP_USAGE(name)				\
> +do {									\
> +	u32 __leaf = __feature_leaf(X86_FEATURE_##name);		\
> +									\
> +	BUILD_BUG_ON(__leaf != kvm_cpu_cap_init_in_progress);		\
> +} while (0)
>  
> -	/*
> -	 * For kernel-defined leafs, mask the boot CPU's pre-populated value.
> -	 * For KVM-defined leafs, explicitly set the leaf, as KVM is the one
> -	 * and only authority.
> -	 */
> -	if (leaf < NCAPINTS)
> -		kvm_cpu_caps[leaf] &= mask;
> -	else
> -		kvm_cpu_caps[leaf] = mask;
> -
> -	kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid);
> -}
> +/*
> + * For kernel-defined leafs, mask the boot CPU's pre-populated value.  For KVM-
> + * defined leafs, explicitly set the leaf, as KVM is the one and only authority.
> + */
> +#define kvm_cpu_cap_init(leaf, mask)					\
> +do {									\
> +	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);	\
> +	const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf;	\

Why not to #define the kvm_cpu_cap_init_in_progress as well instead of a variable?

> +									\
> +	if (leaf < NCAPINTS)						\
> +		kvm_cpu_caps[leaf] &= (mask);				\
> +	else								\
> +		kvm_cpu_caps[leaf] = (mask);				\
> +									\
> +	kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid);			\
> +} while (0)
>  
>  /*
>   * Undefine the MSR bit macro to avoid token concatenation issues when
> @@ -870,6 +889,10 @@ void kvm_set_cpu_caps(void)
>  }
>  EXPORT_SYMBOL_GPL(kvm_set_cpu_caps);
>  
> +#undef kvm_cpu_cap_init
> +#undef KVM_VALIDATE_CPU_CAP_USAGE
> +#define KVM_VALIDATE_CPU_CAP_USAGE(name)
> +
>  struct kvm_cpuid_array {
>  	struct kvm_cpuid_entry2 *entries;
>  	int maxnent;


Best regards,
	Maxim Levitsky