On 01/05/2019 15:58, Tamas K Lengyel wrote:
> On Wed, May 1, 2019 at 7:45 AM Tamas K Lengyel <ta...@tklengyel.com> wrote:
>> On Wed, May 1, 2019 at 1:50 AM Andrew Cooper <andrew.coop...@citrix.com>
>> 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 <ta...@tklengyel.com>
>>>> Cc: Razvan Cojocaru <rcojoc...@bitdefender.com>
>>>> Cc: Jan Beulich <jbeul...@suse.com>
>>>> Cc: Andrew Cooper <andrew.coop...@citrix.com>
>>>> Cc: Wei Liu <wei.l...@citrix.com>
>>>> Cc: Roger Pau Monne <roger....@citrix.com>
>>>> ---
>>>> 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.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
