linux-kernel - Re: [PATCH] KVM: nSVM: Pull CS.Base from actual VMCB12 for soft int/ex re-injection

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]

Date:   Thu, 21 Jul 2022 14:51:30 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] KVM: nSVM: Pull CS.Base from actual VMCB12 for soft
 int/ex re-injection

On Wed, 2022-07-20 at 21:34 +0000, Sean Christopherson wrote:
> On Wed, Jul 20, 2022, Maciej S. Szmigiero wrote:
> > On 20.07.2022 10:43, Maxim Levitsky wrote:
> > > On Mon, 2022-07-18 at 17:47 +0200, Maciej S. Szmigiero wrote:
> > > > Fixes: 6ef88d6e36c2 ("KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction")
> > > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@...cle.com>
> > > > ---
> > > >   arch/x86/kvm/svm/nested.c | 9 +++++----
> > > >   1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > index adf4120b05d90..23252ab821941 100644
> > > > --- a/arch/x86/kvm/svm/nested.c
> > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > @@ -639,7 +639,8 @@ static bool is_evtinj_nmi(u32 evtinj)
> > > >   }
> > > >   static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
> > > > -                                         unsigned long vmcb12_rip)
> > > > +                                         unsigned long vmcb12_rip,
> > > > +                                         unsigned long vmcb12_csbase)
> > > 
> > > Honestly I don't like that nested_vmcb02_prepare_control starts to grow its parameter list,
> > > because it kind of defeats the purpose of vmcb12 cache we added back then.
> > > 
> > > I think that it is better to add csbase/rip to vmcb_save_area_cached,
> > > but I am not 100% sure. What do you think?
> > 
> > This function has only 3 parameters now, so they fit well into registers
> > without taking any extra memory (even assuming it won't get inlined).
> > 
> > If in the future more parameters need to be added to this function
> > (which may or may not happen) then they all can be moved to, for example,
> > vmcb_ctrl_area_cached.
> 
> I don't think Maxim is concerned about the size, rather that we have a dedicated
> struct for snapshotting select save state and aren't using it.

Yes my thoughts exactly. The thing is that passing these values as parameters to this function
also works like a cache, but not going through the cache that we already have
for this purpose.

Anyway for now let it be, but we might need to rethink it in the future.

Best regards,
	Maxim Levitsky

> 
> IIRC, I deliberately avoided using the "cache" because the main/original purpose
> of the cache is to avoid TOCTOU issues.  And because RIP and CS.base aren't checked,
> there's no need to throw them in the cache.