linux-kernel - Re: [PATCH] KVM: nVMX: Always use TLB_FLUSH

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: <CAJD7tkbHARZSUNmoKjax=DHUioP1XBWhf639=7twYC63Dq0vwg@mail.gmail.com>
Date: Thu, 16 Jan 2025 07:25:41 -0800
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Jim Mattson <jmattson@...gle.com>
Cc: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: nVMX: Always use TLB_FLUSH_GUEST for nested VM-Enter/VM-Exit

On Wed, Jan 15, 2025 at 9:27 PM Jim Mattson <jmattson@...gle.com> wrote:
>
> On Wed, Jan 15, 2025 at 7:50 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> >
> > nested_vmx_transition_tlb_flush() uses KVM_REQ_TLB_FLUSH_CURRENT to
> > flush the TLB if VPID is enabled for both L1 and L2, but they still
> > share the TLB tag. This happens if EPT is disabled and KVM fails to
> > allocate a VPID for L2, so both the EPTP and VPID are shared between L1
> > and L2.
>
> Nit: Combined and guest-physical TLB tags are based on EPTRTA (the new
> acronym for EP4TA), not EPTP. But, in any case, with EPT disabled,
> there are no combined or guest-physical mappings. There are only
> linear mappings.

Interestingly, I did initially write EPTRTA, but I changed it to EPTP
because that is the terminology used in nested_has_guest_tlb_tag().
Anyway, I definitely don't mind changing it to EPTRTA.

>
> > Interestingly, nested_vmx_transition_tlb_flush() uses
> > KVM_REQ_TLB_FLUSH_GUEST to flush the TLB for all other cases where a
> > flush is required.
> >
> > Taking a close look at vmx_flush_tlb_guest() and
> > vmx_flush_tlb_current(), the main differences are:
> > (a) vmx_flush_tlb_current() is a noop if the KVM MMU is invalid.
> > (b) vmx_flush_tlb_current() uses INVEPT if EPT is enabled (instead of
> > INVVPID) to flush the guest-physical mappings as well as combined
> > mappings.
> >
> > The check in (a) is seemingly an optimization, and there should not be
> > any TLB entries for L1 anyway if the KVM MMU is invalid. Not having this
> > check in vmx_flush_tlb_guest() is not a fundamental difference, and it
> > can be added there separately if needed.
> >
> > The difference in (b) is irrelevant in this case, because EPT being
> > enabled for L1 means that its TLB tags are tagged with EPTP and cannot
> > be used by L2 (regardless of whether or not EPT is enabled for L2).
>
> The difference is also irrelevant because, as you concluded in the
> first paragraph, EPT is disabled in the final block of
> nested_vmx_transition_tlb_flush().

I was trying to explain that even if EPT is enabled, sharing
guest-physical translations between L1 and L2 should never be possible
(and hence we should never worry about flushing these translations in
nested_vmx_transition_tlb_flush()).  Now that I read it again it is
not as clear as I had hoped.

>
> > Use KVM_REQ_TLB_FLUSH_GUEST in this case in
> > nested_vmx_transition_tlb_flush() for consistency. This arguably makes
> > more sense conceptually too -- L1 and L2 cannot share the TLB tag for
> > guest-physical translations, so only flushing linear and combined
> > translations (i.e. guest-generated translations) is needed.
>
> And, as I mentioned above, with EPT disabled, there are no combined or
> guest-physical mappings.
>
> > Signed-off-by: Yosry Ahmed <yosryahmed@...gle.com>
>
> I think the reasoning in the commit message can be cleared up a bit, but...

Agreed :) I am sure Sean will also want changes in the commit message anyway.

>
> Reviewed-by: Jim Mattson <mattson@...gle.com>

Thanks for the quick review!