KVM: nVMX: Erroneous setting of VMX_EPT_AD_ENABLE_BIT in vmcs02 EPT_POINTER

KVM: nVMX: Erroneous setting of VMX_EPT_AD_ENABLE_BIT in vmcs02 EPT_POINTER

[Jim Mattson]

Construct_eptp() sets the VMX_EPT_AD_ENABLE_BIT in the constructed EPT
pointer based on the setting of the enable_ept_ad_bits module
parameter. This function is used for both the vmcs01 EPT pointer and
the vmcs02 EPT pointer.

However, if the EPT_POINTER field of vmcs12 does not have
VMX_EPT_AD_ENABLE_BIT set, then the vmcs02 EPT pointer must not have
this bit set.

Note that VMX_EPT_AD_ENABLE_BIT does not only enable accessed and
dirty flags in the extended page tables; it also changes the access
mode for page walks in non-root mode from read to write.

Consider the following scenario:

L1 does not set VMX_EPT_AD_ENABLE_BIT in the vmcs12 EPT pointer.
L1 prepares EPT tables for L2 such that only read access is allowed
for the root of L2's x86 page tables (i.e. the GUEST_CR3 in vmcs12).

If L0 sets the VMX_EPT_AD_ENABLE_BIT in the vmcs02 EPT pointer, then
any TLB miss encountered while executing L2 will result in an EPT
violation when the CPU tries to walk L2's page tables (write access to
a write protected page). However, this EPT violation cannot be
forwarded to L1, because L1's virtual CPU would not have delivered it.
(L1's virtual CPU would have performed a read access rather than a
write access.)

With this configuration, L0 will have to emulate each and every L2 instruction.

Better would be for L0 to set the VMX_EPT_AD_ENABLE_BIT in vmcs02 to
match the VMX_EPT_AD_ENABLE_BIT in vmcs12.

Of course, this means that L0 will lose the ability to do
accessed/dirty page tracking of L2 using the shadow EPT tables for L2.

Re: KVM: nVMX: Erroneous setting of VMX_EPT_AD_ENABLE_BIT in vmcs02 EPT_POINTER

[Paolo Bonzini]

On 18/08/2016 20:00, Jim Mattson wrote:
> If L0 sets the VMX_EPT_AD_ENABLE_BIT in the vmcs02 EPT pointer, then
> any TLB miss encountered while executing L2 will result in an EPT
> violation when the CPU tries to walk L2's page tables (write access to
> a write protected page). However, this EPT violation cannot be
> forwarded to L1, because L1's virtual CPU would not have delivered it.
> (L1's virtual CPU would have performed a read access rather than a
> write access.)
> 
> With this configuration, L0 will have to emulate each and every L2 instruction.
> 
> Better would be for L0 to set the VMX_EPT_AD_ENABLE_BIT in vmcs02 to
> match the VMX_EPT_AD_ENABLE_BIT in vmcs12.

And in fact, because we don't expose the feature at all, vmcs02 should
never set the bit.

Would this fix a failure in kvm-unit-tests x86/vmx.c, too?

> Of course, this means that L0 will lose the ability to do
> accessed/dirty page tracking of L2 using the shadow EPT tables for L2.

Indeed, and that's the reason why I never got the courage to look into a
fix for that vmx.c failure...  But maybe it would be enough to ensure
the A/D bits are set when FNAME(sync_page) calls set_spte (accessed is
set if speculative==false; for dirty you'd have to invent a new argument
or something like that).

Paolo

Re: KVM: nVMX: Erroneous setting of VMX_EPT_AD_ENABLE_BIT in vmcs02 EPT_POINTER

[Jim Mattson]

On Fri, Aug 19, 2016 at 3:21 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 18/08/2016 20:00, Jim Mattson wrote:
>> If L0 sets the VMX_EPT_AD_ENABLE_BIT in the vmcs02 EPT pointer, then
>> any TLB miss encountered while executing L2 will result in an EPT
>> violation when the CPU tries to walk L2's page tables (write access to
>> a write protected page). However, this EPT violation cannot be
>> forwarded to L1, because L1's virtual CPU would not have delivered it.
>> (L1's virtual CPU would have performed a read access rather than a
>> write access.)
>>
>> With this configuration, L0 will have to emulate each and every L2 instruction.
>>
>> Better would be for L0 to set the VMX_EPT_AD_ENABLE_BIT in vmcs02 to
>> match the VMX_EPT_AD_ENABLE_BIT in vmcs12.
>
> And in fact, because we don't expose the feature at all, vmcs02 should
> never set the bit.

Without exposing the feature, setting the bit should result in a
VM-entry failure (per section 26.2.1.1 of the Intel SDM, volume 3),
but I don't see any checks on the vmcs12 ept_pointer field.

>
> Would this fix a failure in kvm-unit-tests x86/vmx.c, too?

Possibly. Which failure?

>
>> Of course, this means that L0 will lose the ability to do
>> accessed/dirty page tracking of L2 using the shadow EPT tables for L2.
>
> Indeed, and that's the reason why I never got the courage to look into a
> fix for that vmx.c failure...  But maybe it would be enough to ensure
> the A/D bits are set when FNAME(sync_page) calls set_spte (accessed is
> set if speculative==false; for dirty you'd have to invent a new argument
> or something like that).

Dirty should probably be set any time that the shadow EPT entry has
write permission. Then, we would only want to set write permission in
the shadow EPT entry if the L0 and L1 EPT entries are writable *and*
the current access is a write.

Re: KVM: nVMX: Erroneous setting of VMX_EPT_AD_ENABLE_BIT in vmcs02 EPT_POINTER

[Paolo Bonzini]

On 19/08/2016 16:59, Jim Mattson wrote:
> On Fri, Aug 19, 2016 at 3:21 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Would this fix a failure in kvm-unit-tests x86/vmx.c, too?
> 
> Possibly. Which failure?

This one (didn't have access to the machine with eptad this morning):

Test suite: EPT framework
FAIL: EPT violation - paging structure

It can currently be worked around with eptad=0.  See 28.2.3.2 and 28.4
in the SDM, the latter saying: "When accessed and dirty flags for EPT
are enabled, processor accesses to guest paging-structure entries are
treated as writes (see Section 28.2.3.2)" while the test expects that
guest paging-structure entries can be marked read-only in EPT page tables.

There is another which is unrelated and caused by APICv:

FAIL: EPT - MMIO access

>>
>>> Of course, this means that L0 will lose the ability to do
>>> accessed/dirty page tracking of L2 using the shadow EPT tables for L2.
>>
>> Indeed, and that's the reason why I never got the courage to look into a
>> fix for that vmx.c failure...  But maybe it would be enough to ensure
>> the A/D bits are set when FNAME(sync_page) calls set_spte (accessed is
>> set if speculative==false; for dirty you'd have to invent a new argument
>> or something like that).
> 
> Dirty should probably be set any time that the shadow EPT entry has
> write permission. Then, we would only want to set write permission in
> the shadow EPT entry if the L0 and L1 EPT entries are writable *and*
> the current access is a write.

That basically means behaving as if shadow_accessed_mask ==
shadow_dirty_mask == 0.  I guess one could call kvm_mmu_set_mask_ptes in
nested_ept_{,un}init_mmu_context.

Paolo