| 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
| ||
|
Message-ID: <2ead7655dbb90d15b883c5d31f582ed87d4e8737.camel@redhat.com>
Date: Mon, 21 Nov 2022 13:10:06 +0200
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
linux-kernel@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>,
Sandipan Das <sandipan.das@....com>,
Daniel Sneddon <daniel.sneddon@...ux.intel.com>,
Jing Liu <jing2.liu@...el.com>,
Josh Poimboeuf <jpoimboe@...nel.org>,
Wyes Karny <wyes.karny@....com>,
Borislav Petkov <bp@...en8.de>,
Babu Moger <babu.moger@....com>,
Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
Jim Mattson <jmattson@...gle.com>, x86@...nel.org
Subject: Re: [PATCH 04/13] KVM: nSVM: clean up copying of int_ctl fields
back to vmcb01/vmcb12
On Thu, 2022-11-17 at 20:15 +0000, Sean Christopherson wrote:
> On Thu, Nov 17, 2022, Maxim Levitsky wrote:
> > Clean up the nested_sync_int_ctl_from_vmcb02:
> >
> > 1. The comment about preservation of V_IRQ is wrong: when the L2 doesn't
> > use virtual interrupt masking, then the field just doesn't exist in
> > vmcb12 thus it should not be touched at all.
> > Since it is unused in this case, touching it doesn't matter that much,
> > so the bug is theoretical.
> >
> > 2. When the L2 doesn't use virtual interrupt masking, then in the *theory*
> > if KVM uses the feature, it should copy the changes to V_IRQ* bits from
> > vmcb02 to vmcb01.
> >
> > In practise, KVM only uses it for detection of the interrupt window,
> > and it happens to re-open it on each nested VM exit because
> > kvm_set_rflags happens to raise the KVM_REQ_EVENT.
> > Do this explicitly.
> >
> > 3. Add comment on why we don't need to copy V_GIF from vmcb02 to vmcb01
> > when nested guest doesn't use nested V_GIF (and thus L1's GIF is in
> > vmcb02 while nested), even though it can in theory affect L1's GIF.
> >
> > 4. Add support code to also copy some bits of int_ctl from
> > vmcb02 to vmcb01.
> > Currently there are none.
>
> Unless it's impossible for whatever reason, this patch should be split into
> multiple patches. IIUC, there are at least 2 different functional changes being
> made, they just happen to not have any actual impact on things.
No objection to this.
>
> > No (visible) functional change is intended.
> >
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> > arch/x86/kvm/svm/nested.c | 47 ++++++++++++++++++++++++++-------------
> > 1 file changed, 32 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 54eb152e2b60b6..1f2b8492c8782f 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -410,28 +410,45 @@ void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
> > static void nested_sync_int_ctl_from_vmcb02(struct vcpu_svm *svm,
> > struct vmcb *vmcb12)
> > {
> > - u32 mask;
> > + struct vmcb *vmcb02 = svm->nested.vmcb02.ptr;
> > + struct vmcb *vmcb01 = svm->vmcb01.ptr;
> > +
> > + /* bitmask of bits of int_ctl that we copy from vmcb02 to vmcb12*/
> > + u32 l2_to_l1_mask = 0;
> > + /* bitmask of bits of int_ctl that we copy from vmcb02 to vmcb01*/
> > + u32 l2_to_l0_mask = 0;
> >
> > - /* Only a few fields of int_ctl are written by the processor. */
>
> Can this comment be kept in some form? I found it super useful when reading this
> code just now.
No problem.
>
> > - mask = V_IRQ_MASK | V_TPR_MASK;
> > - if (!(svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) &&
> > - svm_is_intercept(svm, INTERCEPT_VINTR)) {
> > + if (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK)
> > + l2_to_l1_mask |= V_IRQ_MASK | V_TPR_MASK;
> > + else {
> > /*
> > - * In order to request an interrupt window, L0 is usurping
> > - * svm->vmcb->control.int_ctl and possibly setting V_IRQ
> > - * even if it was clear in L1's VMCB. Restoring it would be
> > - * wrong. However, in this case V_IRQ will remain true until
> > - * interrupt_window_interception calls svm_clear_vintr and
> > - * restores int_ctl. We can just leave it aside.
> > + * If IRQ window was opened while in L2, it must be reopened
> > + * after the VM exit
> > + *
> > + * vTPR value doesn't need to be copied from vmcb02 to vmcb01
> > + * because it is synced from/to apic registers on each VM exit
> > */
> > - mask &= ~V_IRQ_MASK;
> > + if (vmcb02->control.int_ctl & V_IRQ_MASK)
> > + kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> > }
> >
> > if (nested_vgif_enabled(svm))
> > - mask |= V_GIF_MASK;
> > + l2_to_l1_mask |= V_GIF_MASK;
> > + else
> > + /* There is no need to sync V_GIF from vmcb02 to vmcb01
> > + * because GIF is cleared on VMexit, thus even though
> > + * nested guest can control host's GIF, on VM exit
> > + * its set value is lost
> > + */
> > + ;
>
> The "else ... ;" is unnecessary, just throw the block comment above the nested
> vGIF if-statment, e.g. if I'm understanding everything, this?
Yes.
>
> /*
> * If nested vGIF is not enabled, L2 has access to L1's "real" GIF. In
> * this case, there's no need to sync V_GIF from vmcb02 to vmcb01
> * because GIF is cleared on VM-Exit, thus any changes made by L2 are
> * overwritten on VM-Exit to L1.
> */
> if (nested_vgif_enabled(svm))
> l2_to_l1_mask |= V_GIF_MASK;
>
> > +
> > + vmcb12->control.int_ctl =
> > + (svm->nested.ctl.int_ctl & ~l2_to_l1_mask) |
> > + (vmcb02->control.int_ctl & l2_to_l1_mask);
> >
> > - vmcb12->control.int_ctl &= ~mask;
> > - vmcb12->control.int_ctl |= svm->vmcb->control.int_ctl & mask;
> > + vmcb01->control.int_ctl =
> > + (vmcb01->control.int_ctl & ~l2_to_l0_mask) |
> > + (vmcb02->control.int_ctl & l2_to_l0_mask);
>
> No need for wrapping immediately after the "=", these all fit under the soft limit:
>
> vmcb12->control.int_ctl = (svm->nested.ctl.int_ctl & ~l2_to_l1_mask) |
> (vmcb02->control.int_ctl & l2_to_l1_mask);
>
> vmcb01->control.int_ctl = (vmcb01->control.int_ctl & ~l2_to_l0_mask) |
> (vmcb02->control.int_ctl & l2_to_l0_mask);
OK.
Best regards,
Maxim Levitsky
>
Powered by blists - more mailing lists