linux-kernel - Re: [PATCH v10 105/108] KVM: TDX: Add methods to ignore accesses to CPU state

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:   Mon, 19 Dec 2022 10:46:57 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>
CC:     "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "dmatlack@...gle.com" <dmatlack@...gle.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>
Subject: Re: [PATCH v10 105/108] KVM: TDX: Add methods to ignore accesses to
 CPU state

On Thu, 2022-12-15 at 21:26 -0800, Isaku Yamahata wrote:
> On Wed, Dec 14, 2022 at 11:43:14AM +0000,
> "Huang, Kai" <kai.huang@...el.com> wrote:
> 
> > On Sat, 2022-10-29 at 23:23 -0700, isaku.yamahata@...el.com wrote:
> > > +static u8 vt_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> > > +{
> > > +	if (is_td_vcpu(vcpu)) {
> > > +		if (is_mmio)
> > > +			return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
> > > +		return  MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;
> > > +	}
> > > +
> > > +	return vmx_get_mt_mask(vcpu, gfn, is_mmio);
> > > +}
> > 
> > So you are returning WB for _ALL_ guest memory, including shared.  Wouldn't this
> > break MTRR handling for shared memory?  For instance, IIUC we can still support
> > assigning a device to a TDX guest while the VT-d doesn't support coherent
> > memory, in which case guest's MTRR/PAT are honored.  I think this should also
> > apply to TDX guest's shared memory?
> 
> You're right. So here is the updated change.
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -798,11 +798,8 @@ static int vt_set_identity_map_addr(struct kvm *kvm, u64 ident_addr)
>  
>  static u8 vt_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
>  {
> -       if (is_td_vcpu(vcpu)) {
> -               if (is_mmio)
> -                       return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
> -               return  MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;
> -       }
> +       if (is_td_vcpu(vcpu))
> +               return tdx_get_mt_mask(vcpu, gfn, is_mmio);
>  
>         return vmx_get_mt_mask(vcpu, gfn, is_mmio);
>  }
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index ac47b20e4e91..f1842eb32d6c 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -568,7 +568,31 @@ int tdx_vm_init(struct kvm *kvm)
>         return 0;
>  }
>  
> +u8 tdx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> +{
> +       if (is_mmio)
> +               return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
> +
> +       if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
> +               return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
> +
> +       /* Guest CR0 is unknown.  Assume CR0.CD = 0. */

This comment is horrible.  You need to explain why guest CR0.CD is irrelevant
here.

And the fact is for TDX guest, TDX module tells us the CR0.CD is always 0.

> +
> +       /* TDX private GPA is always WB. */
> +       if (gfn & kvm_gfn_shared_mask(vcpu->kvm))
> +               return  MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;

Dose the 'gfn' still have the shared bit set?  I don't think so?

> +
> +       /*
> +        * Because the definition of MTRR MSR is unaware of shared-bit,
> +        * clear shared-bit.
> +        */
> +       gfn = kvm_gfn_private(vcpu->kvm, gfn);
> +       return kvm_mtrr_get_guest_memory_type(vcpu, gfn) << VMX_EPT_MT_EPTE_SHIFT;
> +}

Overall, I think a better logic should be: 1) for private page, return WB (and
WARN_ON(is_mmio)), although this doesn't matter anyway as the SPTE won't be used
by CPU anyway. 2) Then for shared pages, we try to copy vmx_get_mt_mask() logic,
or use vmx_get_mt_mask() directly.

But since how KVM and TDX module virtualize MTRR/PAT is different, thus for now
I am not sure whether just using vmx_get_mt_mask() can work.  For example, IIUC
for TDX guest KVM cannot know guest's PAT, but for VMX guest KVM traps MTRR/PAT
so it can know guest's memory type.

For now I am not even sure whether we can/should support device assignment w/o
VT-d snooping capability as for TDX guest KVM cannot know guest's PAT?  Perhaps
we should just make sure this won't happen so we can always returns WB for
shared memory too for TDX guest.