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

[Maciej S. Szmigiero]

On 20.07.2022 10:43, Maxim Levitsky wrote:
> On Mon, 2022-07-18 at 17:47 +0200, Maciej S. Szmigiero wrote:
> > From: "Maciej S. Szmigiero" <maciej.szmigiero@xxxxxxxxxx>
> >
> > enter_svm_guest_mode() first calls nested_vmcb02_prepare_control() to copy
> > control fields from VMCB12 to the current VMCB, then
> > nested_vmcb02_prepare_save() to perform a similar copy of the save area.
> >
> > This means that nested_vmcb02_prepare_control() still runs with the
> > previous save area values in the current VMCB so it shouldn't take the L2
> > guest CS.Base from this area.
> >
> > Explicitly pull CS.Base from the actual VMCB12 instead in
> > enter_svm_guest_mode().
> >
> > Granted, having a non-zero CS.Base is a very rare thing (and even
> > impossible in 64-bit mode), having it change between nested VMRUNs is
> > probably even rarer, but if it happens it would create a really subtle bug
> > so it's better to fix it upfront.
> >
> > Fixes: 6ef88d6e36c2 ("KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction")
> > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@xxxxxxxxxx>
> > ---
> >   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.

> Best regards,
> 	Maxim Levitsky

Thanks,
Maciej