Re: [PATCH] x86/shadow: Drop incorrect diagnostic when shadowing TSS.RSP0

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/04/2019 13:11, Jan Beulich wrote:
>>>> On 08.04.19 at 13:37, <andrew.cooper3@citrix.com> wrote:
>> On 08/04/2019 11:14, Jan Beulich wrote:
>>>>>> On 05.04.19 at 21:09, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>> @@ -3305,8 +3305,9 @@ static int sh_page_fault(struct vcpu *v,
>>>>      {
>>>>          /*
>>>>           * If we are in the middle of injecting an exception or interrupt then
>>>> -         * we should not emulate: it is not the instruction at %eip that caused
>>>> -         * the fault. Furthermore it is almost certainly the case the handler
>>>> +         * we should not emulate: the fault is a side effect of the processor
>>>> +         * trying to push an exception frame onto a stack which has yet to be
>>>> +         * shadowed.  Furthermore it is almost certainly the case the handler
>>>>           * stack is currently considered to be a page table, so we should
>>>>           * unshadow the faulting page before exiting.
>>>>           */
>>> Your addition to me looks to contradict the part of the comment you
>>> leave in place: You say "which has yet to be shadowed", while the
>>> pre-existing text says "it is almost certainly the case the handler
>>> stack is currently considered to be a page table", which to me means
>>> it _is_ already shadowed (and in fact should not be).
>>>
>>> In your addition, do you perhaps mean the page tables covering the
>>> stack which have yet to be shadowed?
>> This clause is inside an hvm_event_pending() which looks at VMCS/VMCB
>> pending injection.
>>
>> This only becomes true via VT-x's
>>
>>     __vmread(IDT_VECTORING_INFO, &idtv_info);
>>     if ( exit_reason != EXIT_REASON_TASK_SWITCH )
>>         vmx_idtv_reinject(idtv_info);
>>
>> path, and the equivalent case on SVM which leaves the EVENTINJ field
>> valid after vmexit.  (This is assuming that we have no bugs whereby we
>> enter sh_page_fault() late, after some emulation has occurred.)
>>
>> What this means is that the processor is trying to deliver an exception,
>> and the #PF intercept has been hit (which occurs before escalation to
>> #DF).  i.e. it is the memory reads/writes made by microcode which suffer
>> a fault due to the linear addresses not being present in the shadows.
>>
>> Beyond that, there is a second aspect to getting here, which is when the
>> linear address hit something which the shadow code thinks is protected,
>> which AFAICT, starts off as everything which doesn't have an L1 shadow
>> pointing writeably at it.
>>
>> In the XTF case where I encountered this first, it so happens that the
>> processor delivering an exception from userspace is the first thing to
>> ever touch the linear address at RSP0, so the stack always becomes
>> shadowed during IDT vectoring.
> I'm (at least) mildly confused: I follow what you write (I think), but
> you again say "the stack always becomes shadowed". My original
> question was whether you really mean that, as stacks, if at all,
> should get shadowed only unintentionally (and hence get un-shadowed
> immediately when that condition is detected). That is, my (slightly
> rephrased) question stands: Do you perhaps mean the page tables
> mapping the stack to become shadowed, rather than the stack itself?

I guess this is an issue of terminology, to which I defer to Tim to judge.

But yes - I mean is "the linear address mapping RSP0 getting entered
into the shadow pagetables".

~Andrew

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