Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 23, 2024, mlevitsk@redhat.com wrote:
> У ср, 2024-08-21 у 09:04 -0700, Sean Christopherson пише:
> > > static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu,
> > >                                            unsigned int flags)
> > > {
> > >         if (flags & (X86EMUL_F_INVLPG | X86EMUL_F_MSR | X86EMUL_F_DT_LOAD))
> > >                 return !__is_canonical_address(la, max_host_virt_addr_bits());
> > >         else
> > >                 return !__is_canonical_address(la, vcpu_virt_addr_bits(vcpu));
> > > }
> 
> This can work in principle, although are you OK with using these emulator flags
> outside of the emulator code?

Yep, they're already used in VMX's vmx_get_untagged_addr().  

> I am asking because the is_noncanonical_address is used in various places across KVM.

...

> > > We wouldn't want wrapper for everything, e.g. to minimize the risk of creating a
> > > de factor implicit default, but I think those three, and maybe a code/fetch
> > > variant, will cover all but a few users.
> > > 
> > > > > About fixing the emulator this is what see:
> > > > > 
> > > > >         emul_is_noncanonical_address
> > > > >                 __load_segment_descriptor
> > > > >                         load_segment_descriptor
> > > > >                                 em_lldt
> > > > >                                 em_ltr
> > > > > 
> > > > >                 em_lgdt_lidt
> > > > > 
> > > > > 
> > > > > 
> > > > > While em_lgdt_lidt should be easy to fix because it calls
> > > > > emul_is_noncanonical_address directly,
> > > 
> > > Those don't need to be fixed, they are validating the memory operand, not the
> > > base of the descriptor, i.e. aren't exempt from CR4.LA57.
> 
> Are you sure?

Nope.  Re-reading what I wrote, I have no idea what code past me was looking at.

I even contradicted myself later on:

  So the explicit emul_is_noncanonical_address() check in __load_segment_descriptor()
  needs to be tagged X86EMUL_F_BASE, but otherwise it all should Just Work (knock wood).

so yeah, ignore this.

> em_lgdt_lidt reads its memory operands (and checks that it is canonical through
> linearize) with read_descriptor and that is fine because this is memory fetch, 
> and then it checks that the base address within the operand is canonical.
> 
> This check needs to be updated, as it is possible to load non canonical GDT and IDT
> base via lgdt/lidt (I tested this).
> 
> For em_lldt, em_ltr, the check on the system segment descriptor base is
> in __load_segment_descriptor:
> 
> ...
>  ret = linear_read_system(ctxt, desc_addr+8, &base3, sizeof(base3));
>  if (ret != X86EMUL_CONTINUE)
>  return ret;
>  if (emul_is_noncanonical_address(get_desc_base(&seg_desc) |
>  ((u64)base3 << 32), ctxt))
>  return emulate_gp(ctxt, err_code);
> 
> ...
> 
> 
> 64 bases are possible only for system segments, which are
> TSS, LDT, and call gates/IDT descriptors.
> 
> 
> We don't emulate IDT fetches in protected mode, and as I found out the hard way after
> I wrote a unit test to do a call through a call gate, the emulator doesn't
> support call gates either)
> 
> Thus I can safely patch __load_segment_descriptor.

Agreed.

And thinking more about how this is likely implemented in ucode, this is probably
working as intended.  The the SDM gives CPUs a _lot_ of leeway:

  In 64-bit mode, an address is considered to be in canonical form if address
  bits 63 through to the most-significant implemented bit by the microarchitecture
  are set to either all ones or all zeros.

as does the APM:

  Long mode defines 64 bits of virtual address, but implementations of the AMD64
  architecture may support fewer bits of virtual address. Although implementations
  might not use all 64 bits of the virtual address, they check bits 63 through the
  most-significant implemented bit to see if those bits are all zeros or all ones.
  An address that complies with this property is said to be in canonical address
  form. If a virtual-memory reference is not in canonical form, the implementation
  causes a general-protection exception or stack fault.

I suspect that CR4.LA_57 is only consulted when the CPU is actually consuming the
address, i.e. is (or is about to, e.g. for code fetches) generating a memory
access.

Heh, and for MPX, the SDM kinda sorta confirms that LA57 is ignored, though I
doubt the author of this section intended their words to be taken this way :-)

  WRMSR to BNDCFGS will #GP if any of the reserved bits of BNDCFGS is not zero or
  if the base address of the bound directory is not canonical. XRSTOR of BNDCFGU
  ignores the reserved bits and does not fault if any is non-zero; similarly, it
  ignores the upper bits of the base address of the bound directory and sign-extends
  the highest implemented bit of the linear address to guarantee the canonicality
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  of this address.

> > > There _is_ an indirect canonical check on the _descriptor_ (the actual descriptor
> > > pointed at by the selector, not the memory operand).  The SDM is calls this case
> > > out in the LFS/LDS docs:
> > > 
> > >   If the FS, or GS register is being loaded with a non-NULL segment selector and
> > >   any of the following is true: the segment selector index is not within descriptor
> > >   table limits, the memory address of the descriptor is non-canonical
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I tested that both ltr and lldt do ignore CR4.LA57 by loading from a descriptor
> which had a non canonical value and CR4.LA57 clear.