Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <8018a697eae37a5e749afcb19368ac7c32b91d57.camel@redhat.com>
Subject: Re: [PATCH 1/2] Revert "nSVM: Check for reserved encodings of
 TLB_CONTROL in nested VMCB"
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Stefan Sterz <s.sterz@proxmox.com>
Date:   Mon, 20 Nov 2023 13:41:13 +0200
In-Reply-To: <20231018194104.1896415-2-seanjc@google.com>
References: <20231018194104.1896415-1-seanjc@google.com>
         <20231018194104.1896415-2-seanjc@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@proxmox.com>
> Closes: https://lkml.kernel.org/r/b9915c9c-4cf6-051a-2d91-44cc6380f455%40proxmox.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.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@redhat.com>

Best regards,
	Maxim Levitsky