Re: [Xen-devel] [PATCH] x86/vm_event: correctly gather gs_shadow value

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, May 1, 2019 at 11:03 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 01/05/2019 15:58, Tamas K Lengyel wrote:
> > On Wed, May 1, 2019 at 7:45 AM Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> >> On Wed, May 1, 2019 at 1:50 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> >> wrote:
> >>> On 01/05/2019 05:22, Tamas K Lengyel wrote:
> >>>> Currently the gs_shadow value is only cached when the vCPU is being 
> >>>> scheduled
> >>>> out by Xen. Reporting this (usually) stale value through vm_event is 
> >>>> incorrect,
> >>>> since it doesn't represent the actual state of the vCPU at the time the 
> >>>> event
> >>>> was recorded. This prevents vm_event subscribers from correctly finding 
> >>>> kernel
> >>>> structures in the guest when it is trapped while in ring3.
> >>>>
> >>>> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> >>>> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> >>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >>>> Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> >>>> ---
> >>>>  xen/arch/x86/vm_event.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> >>>> index 51c3493b1d..4464940da7 100644
> >>>> --- a/xen/arch/x86/vm_event.c
> >>>> +++ b/xen/arch/x86/vm_event.c
> >>> Actually, come to think of it, the same is true for the SYSENTER
> >>> details, which by default are read/write to the guest while it is
> >>> scheduled.  As a result, the details reported here will from the last
> >>> vcpu context switch, and possibly stale.
> >> I'll look into it.
> > The sysenter values look fine to me because vmx_save_vmcs_ctxt calls
> > vmx_vmcs_save, which refreshes the values from the actual VMCS. It's
> > only shadow_gs that is not refreshed.
>
> So, you are correct that we call into vmx_vmcs_save() which causes the
> SYSENTER details to be correct.
>
> However, the same path also calls vmx_save_cpu_state() which saves
> shadow_gs, and therefore ought to DTRT for the previous use in vm_event.
>
> The problem is that vmx_{save,load}_cpu_state() only function correctly
> in remote context, which is why the stale shadow_gs persists.
>
> Contrary to what I said earlier, I now think that a better fix would be
> to make the above functions safe to use use in current context (at least
> for the save side - the restore side can probably just ASSERT() atm, as
> it doesn't seem to have an equivalent use).
>
> That way, the vm_event code doesn't need to contain any
> context-sensitive code, which is a better overall, IMO.

Sounds good to me. So a "v == current" case to refresh the gs_shadow
value is all this will entail, correct?

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel