Re: [PATCH v6 2/7] KVM: VMX: Use is_64_bit_mode() to check 64-bit mode

From: "Huang, Kai" <kai.huang@intel.com>
To: "Christopherson,, Sean" <seanjc@google.com>,
	"Gao, Chao" <chao.gao@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"robert.hu@linux.intel.com" <robert.hu@linux.intel.com>,
	"binbin.wu@linux.intel.com" <binbin.wu@linux.intel.com>
Subject: Re: [PATCH v6 2/7] KVM: VMX: Use is_64_bit_mode() to check 64-bit mode
Date: Tue, 28 Mar 2023 23:33:46 +0000	[thread overview]
Message-ID: <12c4f1d3c99253f364f3945a998fdccb0ddf300f.camel@intel.com> (raw)
In-Reply-To: <ZBojJgTG/SNFS+3H@google.com>

On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote:
> On Mon, Mar 20, 2023, Chao Gao wrote:
> > On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote:
> > > get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode()
> > > to check 64-bit mode. Should use is_64_bit_mode() instead.
> > > 
> > > Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions")
> > > Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions")
> > 
> > It is better to split this patch into two: one for nested and one for
> > SGX.
> > 
> > It is possible that there is a kernel release which has just one of
> > above two flawed commits, then this fix patch cannot be applied cleanly
> > to the release.
> 
> The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except
> for VMCALL, that #UD has higher priority than VM-Exit interception.  So I'd say
> just drop the nVMX side of things.

But it looks the old code doesn't unconditionally inject #UD when in
compatibility mode?

        /* Checks for #GP/#SS exceptions. */                                   
        exn = false;                                                           
        if (is_long_mode(vcpu)) {                                              
                /*                                                             
                 * The virtual/linear address is never truncated in 64-bit     
                 * mode, e.g. a 32-bit address size can yield a 64-bit virtual 
                 * address when using FS/GS with a non-zero base.              
                 */                                                            
                if (seg_reg == VCPU_SREG_FS || seg_reg == VCPU_SREG_GS)        
                        *ret = s.base + off;                                   
                else                                                           
                        *ret = off;                                            

                /* Long mode: #GP(0)/#SS(0) if the memory address is in a      
                 * non-canonical form. This is the only check on the memory    
                 * destination for long mode!                                  
                 */                                                            
                exn = is_noncanonical_address(*ret, vcpu); 
	}
	...

The logic of only adding seg.base for FS/GS to linear address (and ignoring
seg.base for all other segs) only applies to 64 bit mode, but the code only
checks _long_ mode.

Am I missing something?

> 
> I could have sworn ENCLS had the same behavior, but the SDM disagrees.  Though why
> on earth ENCLS is allowed in compatibility mode is beyond me.  ENCLU I can kinda
> sorta understand, but ENCLS?!?!!

I can reach out to Intel guys to (try to) find the answer if you want me to? :)