linux-kernel - Re: [PATCH] KVM: x86/mmu: Warn on nx_huge

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]

Date:   Tue, 19 Oct 2021 16:23:14 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Li Yu <liyu.yukiteru@...edance.com>
Cc:     pbonzini@...hat.com, Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86/mmu: Warn on nx_huge_pages for possible problems

On Tue, Oct 19, 2021, Li Yu wrote:
> Add warning when `nx_huge_pages` is enabled by `auto` for hint that
> huge pages may be splited by kernel.
> 
> Add warning when CVE-2018-12207 may arise but `nx_huge_pages` is
> disabled for hint that malicious guest may cause a CPU lookup.

For the shortlog and changelog, "warning" is misleading.  A "warn" usually means
a WARN with a backtrace.  This is really just an information message that happens
to be displayed at level=warn, and that's an implementation detail.

> Signed-off-by: Li Yu <liyu.yukiteru@...edance.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 1a64ba5b9437..32026592e566 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6056,19 +6056,26 @@ static void __set_nx_huge_pages(bool val)
>  	nx_huge_pages = itlb_multihit_kvm_mitigation = val;
>  }
>  
> +#define ITLB_MULTIHIT_NX_ON  "iTLB multi-hit CPU bug present and cpu mitigations enabled, huge pages may be splited by kernel for security. See CVE-2018-12207 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/multihit.html for details.\n"
                                                                                            ^                  ^^^^^^^
											    |- guest           split
> +#define ITLB_MULTIHIT_NX_OFF "iTLB multi-hit CPU bug present and cpu mitigations enabled, malicious guest may cause a CPU lookup. See CVE-2018-12207 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/multihit.html for details.\n"
                                                                                    ^^^^^^^
										    disabled

This is almost entirely redundant with the information that is displayed in
/sys/devices/system/cpu/vulnerabilities/itlb_multihit and
/sys/module/kvm/parameters/nx_huge_pages.  It also makes the below helper more
than a bit messy.

I kind of agree with Paolo's feedback that KVM should log a message if the bug
is present but the mitigation is off.  But if the admin explicitly turns off the
mitigation, logging the message every time KVM is loaded is obnoxious.  IMO, if
we want to display a message then we should do something similar to l1tf_mitigation
and give the admin the option to suppress any logging.

Personally, I don't see much value in a message of any kind.  Anyone that is
running untrusted guests should darn well do a lot of performance testing and
risk analysis before touching the knob.  And any use case that cares enough about
performance to explicitly turn off the mitigation should already know exactly how
KVM' params affect performance.

The L1D flushing messages are justified because a data leak is theoretically possible
when SMT is enabled even when the kernel's default mitigation is enabled.  That's not
the case here.

>  static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
>  {
>  	bool old_val = nx_huge_pages;
>  	bool new_val;
>  
>  	/* In "auto" mode deploy workaround only if CPU has the bug. */
> -	if (sysfs_streq(val, "off"))
> +	if (sysfs_streq(val, "off")) {
>  		new_val = 0;
> -	else if (sysfs_streq(val, "force"))
> +		if (get_nx_auto_mode() && new_val != old_val)
> +			pr_warn(ITLB_MULTIHIT_NX_OFF);
> +	} else if (sysfs_streq(val, "force"))
>  		new_val = 1;
> -	else if (sysfs_streq(val, "auto"))
> +	else if (sysfs_streq(val, "auto")) {
>  		new_val = get_nx_auto_mode();
> -	else if (strtobool(val, &new_val) < 0)
> +		if (new_val && new_val != old_val)
> +			pr_warn(ITLB_MULTIHIT_NX_ON);
> +	} else if (strtobool(val, &new_val) < 0)

All branches need braces if any branch has braces.

>  		return -EINVAL;
>  
>  	__set_nx_huge_pages(new_val);
> @@ -6095,8 +6102,11 @@ int kvm_mmu_module_init(void)
>  {
>  	int ret = -ENOMEM;
>  
> -	if (nx_huge_pages == -1)
> +	if (nx_huge_pages == -1) {
>  		__set_nx_huge_pages(get_nx_auto_mode());
> +		if (is_nx_huge_page_enabled())
> +			pr_warn_once(ITLB_MULTIHIT_NX_ON);
> +	}
>  
>  	/*
>  	 * MMU roles use union aliasing which is, generally speaking, an
> -- 
> 2.11.0
>