linux-kernel - Re: [PATCH RFC] KVM: SVM: reduce guest MAXPHYADDR by one in case C-bit is a physical bit

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:   Fri, 15 Oct 2021 15:24:28 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        David Matlack <dmatlack@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] KVM: SVM: reduce guest MAXPHYADDR by one in case
 C-bit is a physical bit

On Fri, Oct 15, 2021, Vitaly Kuznetsov wrote:
> Several selftests (memslot_modification_stress_test, kvm_page_table_test,
> dirty_log_perf_test,.. ) which rely on vm_get_max_gfn() started to fail
> since commit ef4c9f4f65462 ("KVM: selftests: Fix 32-bit truncation of
> vm_get_max_gfn()") on AMD EPYC 7401P:
> 
>  ./tools/testing/selftests/kvm/demand_paging_test
>  Testing guest mode: PA-bits:ANY, VA-bits:48,  4K pages
>  guest physical test memory offset: 0xffffbffff000

This look a lot like the signature I remember from the original bug[1].  I assume
you're hitting the magic HyperTransport region[2].  I thought that was fixed, but
the hack-a-fix for selftests never got applied[3].

[1] https://lore.kernel.org/lkml/20210623230552.4027702-4-seanjc@google.com/
[2] https://lkml.kernel.org/r/7e3a90c0-75a1-b8fe-dbcf-bda16502ace9@amd.com
[3] https://lkml.kernel.org/r/20210805105423.412878-1-pbonzini@redhat.com

>  Finished creating vCPUs and starting uffd threads
>  Started all vCPUs
>  ==== Test Assertion Failure ====
>    demand_paging_test.c:63: false
>    pid=47131 tid=47134 errno=0 - Success
>       1	0x000000000040281b: vcpu_worker at demand_paging_test.c:63
>       2	0x00007fb36716e431: ?? ??:0
>       3	0x00007fb36709c912: ?? ??:0
>    Invalid guest sync status: exit_reason=SHUTDOWN
> 
> The commit, however, seems to be correct, it just revealed an already
> present issue. AMD CPUs which support SEV may have a reduced physical
> address space, e.g. on AMD EPYC 7401P I see:
> 
>  Address sizes:  43 bits physical, 48 bits virtual
> 
> The guest physical address space, however, is not reduced as stated in
> commit e39f00f60ebd ("KVM: x86: Use kernel's x86_phys_bits to handle
> reduced MAXPHYADDR"). This seems to be almost correct, however, APM has one
> more clause (15.34.6):
> 
>   Note that because guest physical addresses are always translated through
>   the nested page tables, the size of the guest physical address space is
>   not impacted by any physical address space reduction indicated in CPUID
>   8000_001F[EBX]. If the C-bit is a physical address bit however, the guest
>   physical address space is effectively reduced by 1 bit.
> 
> Implement the reduction.
> 
> Fixes: e39f00f60ebd (KVM: x86: Use kernel's x86_phys_bits to handle reduced MAXPHYADDR)
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---
> - RFC: I may have misdiagnosed the problem as I didn't dig to where exactly
>  the guest crashes.
> ---
>  arch/x86/kvm/cpuid.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 751aa85a3001..04ae280a0b66 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -923,13 +923,20 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		 *
>  		 * If TDP is enabled but an explicit guest MAXPHYADDR is not
>  		 * provided, use the raw bare metal MAXPHYADDR as reductions to
> -		 * the HPAs do not affect GPAs.
> +		 * the HPAs do not affect GPAs. The value, however, has to be
> +		 * reduced by 1 in case C-bit is a physical bit (APM section
> +		 * 15.34.6).
>  		 */
> -		if (!tdp_enabled)
> +		if (!tdp_enabled) {
>  			g_phys_as = boot_cpu_data.x86_phys_bits;
> -		else if (!g_phys_as)
> +		} else if (!g_phys_as) {
>  			g_phys_as = phys_as;
>  
> +			if (kvm_cpu_cap_has(X86_FEATURE_SEV) &&
> +			    (cpuid_ebx(0x8000001f) & 0x3f) < g_phys_as)
> +				g_phys_as -= 1;

This is incorrect, non-SEV guests do not see a reduced address space.  See Tom's
explanation[*]

[*] https://lkml.kernel.org/r/324a95ee-b962-acdf-9bd7-b8b23b9fb991@amd.com

> +		}
> +
>  		entry->eax = g_phys_as | (virt_as << 8);
>  		entry->edx = 0;
>  		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
> -- 
> 2.31.1
>