linux-kernel - Re: [PATCH 1/2] Revert "nSVM: Check for reserved encodings of TLB

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: <8018a697eae37a5e749afcb19368ac7c32b91d57.camel@redhat.com>
Date:   Mon, 20 Nov 2023 13:41:13 +0200
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Stefan Sterz <s.sterz@...xmox.com>
Subject: Re: [PATCH 1/2] Revert "nSVM: Check for reserved encodings of
 TLB_CONTROL in nested VMCB"

On Wed, 2023-10-18 at 12:41 -0700, Sean Christopherson wrote:
> Revert KVM's made-up consistency check on SVM's TLB control.  The APM says
> that unsupported encodings are reserved, but the APM doesn't state that
> VMRUN checks for a supported encoding.  Unless something is called out
> in "Canonicalization and Consistency Checks" or listed as MBZ (Must Be
> Zero), AMD behavior is typically to let software shoot itself in the foot.
> 
> This reverts commit 174a921b6975ef959dd82ee9e8844067a62e3ec1.
> 
> Fixes: 174a921b6975 ("nSVM: Check for reserved encodings of TLB_CONTROL in nested VMCB")
> Reported-by: Stefan Sterz <s.sterz@...xmox.com>
> Closes: https://lkml.kernel.org/r/b9915c9c-4cf6-051a-2d91-44cc6380f455%40proxmox.com
> Cc: stable@...r.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/svm/nested.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 3fea8c47679e..60891b9ce25f 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -247,18 +247,6 @@ static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa, u32 size)
>  	    kvm_vcpu_is_legal_gpa(vcpu, addr + size - 1);
>  }
>  
> -static bool nested_svm_check_tlb_ctl(struct kvm_vcpu *vcpu, u8 tlb_ctl)
> -{
> -	/* Nested FLUSHBYASID is not supported yet.  */
> -	switch(tlb_ctl) {
> -		case TLB_CONTROL_DO_NOTHING:
> -		case TLB_CONTROL_FLUSH_ALL_ASID:
> -			return true;
> -		default:
> -			return false;
> -	}
> -}
> -
>  static bool __nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
>  					 struct vmcb_ctrl_area_cached *control)
>  {
> @@ -278,9 +266,6 @@ static bool __nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
>  					   IOPM_SIZE)))
>  		return false;
>  
> -	if (CC(!nested_svm_check_tlb_ctl(vcpu, control->tlb_ctl)))
> -		return false;
> -
>  	if (CC((control->int_ctl & V_NMI_ENABLE_MASK) &&
>  	       !vmcb12_is_intercept(control, INTERCEPT_NMI))) {
>  		return false;


Yes, after checking Jim's comment (*) on this I still agree that revert is OK.
KVM never passes through the tlb_ctl field (but does copy it to the cache),
thus there is no need to sanitize it.

https://www.spinics.net/lists/kvm/msg316072.html


Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>

Best regards,
	Maxim Levitsky