linux-kernel - Re: [RESEND PATCH ] KVM: VMX: Enable/disable PML when dirty logging gets enabled/disabled

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] [day] [month] [year] [list]

Message-ID: <YCb9cUwu+7SrM2sq@google.com>
Date:   Fri, 12 Feb 2021 14:13:05 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Makarand Sonare <makarandsonare@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, pshier@...gle.com, jmattson@...gle.com,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [RESEND PATCH ] KVM: VMX: Enable/disable PML when dirty logging
 gets enabled/disabled

On Fri, Feb 12, 2021, Sean Christopherson wrote:
> On Fri, Feb 12, 2021, Makarand Sonare wrote:
> > >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > >> index 777177ea9a35e..eb6639f0ee7eb 100644
> > >> --- a/arch/x86/kvm/vmx/vmx.c
> > >> +++ b/arch/x86/kvm/vmx/vmx.c
> > >> @@ -4276,7 +4276,7 @@ static void
> > >> vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
> > >>  	*/
> > >>  	exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
> > >>
> > >> -	if (!enable_pml)
> > >> +	if (!enable_pml || !vcpu->kvm->arch.pml_enabled)
> > >>  		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
> > >
> > > The checks are unnecessary if PML is dynamically toggled, i.e. this
> > > snippet can unconditionally clear PML.  When setting SECONDARY_EXEC
> > > (below snippet), PML will be preserved in the current controls, which is
> > > what we want.
> > 
> > Assuming a new VCPU can be added at a later time after PML is already
> > enabled, should we clear
> > PML in VMCS for the new VCPU. If yes what will be the trigger for
> > setting PML for the new VCPU?
> 
> Ah, didn't consider that.  Phooey.

I remember why I thought this could be unconditional.  Adding PML to the list of
dynamic bits in vmcs_set_secondary_exec_control() effectively makes this code
unconditional, because it means that current bit will be preserved, including
the case where PML=0 when a vCPU is created.

I believe the fix is simply to not mark PML as fully dynamic.