Re: [Xen-devel] [PATCH 2/2] x86/svm: Write the correct %eip into the outgoing task

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/11/2019 13:59, Roger Pau Monné wrote:
> On Thu, Nov 21, 2019 at 10:15:51PM +0000, Andrew Cooper wrote:
>> The TASK_SWITCH vmexit has fault semantics, and doesn't provide any NRIPs
>> assistance with instruction length.  As a result, any instruction-induced 
>> task
>> switch has the outgoing task's %eip pointing at the instruction switch caused
>                                                                   ^ that
>> the switch, rather than after it.
>>
>> This causes explicit use of task gates to livelock (as when the task returns,
>> it executes the task-switching instruction again), and any restartable task 
>> to
>> become a nop after its first instantiation (the entry state points at the
>> ret/iret instruction used to exit the task).
>>
>> 32bit Windows in particular is known to use task gates for NMI handling, and
>> to use NMI IPIs.
>>
>> In the task switch handler, distinguish instruction-induced from
>> interrupt/exception-induced task switches, and decode the instruction under
>> %rip to calculate its length.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>>
>> The implementation of svm_get_task_switch_insn_len() is bug-compatible with
>> svm_get_insn_len() when it comes to conditional #GP'ing.  I still haven't had
>> time to address this more thoroughly.
>>
>> AMD does permit TASK_SWITCH not to be intercepted and, I'm informed does do
>> the right thing when it comes to a TSS crossing a page boundary.  However, it
>> is not actually safe to leave task switches unintercepted.  Any NPT or shadow
>> page fault, even from logdirty/paging/etc will corrupt guest state in an
>> unrecoverable manner.
>> ---
>>  xen/arch/x86/hvm/svm/emulate.c        | 55 
>> +++++++++++++++++++++++++++++++++++
>>  xen/arch/x86/hvm/svm/svm.c            | 46 ++++++++++++++++++++++-------
>>  xen/include/asm-x86/hvm/svm/emulate.h |  1 +
>>  3 files changed, 92 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
>> index 3e52592847..176c25f60d 100644
>> --- a/xen/arch/x86/hvm/svm/emulate.c
>> +++ b/xen/arch/x86/hvm/svm/emulate.c
>> @@ -117,6 +117,61 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned 
>> int instr_enc)
>>  }
>>  
>>  /*
>> + * TASK_SWITCH vmexits never provide an instruction length.  We must always
>> + * decode under %rip to find the answer.
>> + */
>> +unsigned int svm_get_task_switch_insn_len(struct vcpu *v)
>> +{
>> +    struct hvm_emulate_ctxt ctxt;
>> +    struct x86_emulate_state *state;
>> +    unsigned int emul_len, modrm_reg;
>> +
>> +    ASSERT(v == current);
>> +    hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
>> +    hvm_emulate_init_per_insn(&ctxt, NULL, 0);
>> +    state = x86_decode_insn(&ctxt.ctxt, hvmemul_insn_fetch);
>> +    if ( IS_ERR_OR_NULL(state) )
> Maybe crash the guest in this case? Not advancing the instruction
> pointer in a software induced task switch will create a loop AFAICT?

Your analysis is correct, but crashing the guest would be a user=>kernel
DoS, which is worse than a livelock.

We do have some logic to try and cope with this in svm.c, and I think
I've got a better idea of how to make use of it.

>
>> +        return 0;
>> +
>> +    emul_len = x86_insn_length(state, &ctxt.ctxt);
>> +
>> +    /*
>> +     * Check for an instruction which can cause a task switch.  Any far
>> +     * jmp/call/ret, any software interrupt/exception, and iret.
>> +     */
>> +    switch ( ctxt.ctxt.opcode )
>> +    {
>> +    case 0xff: /* Grp 5 */
>> +        /* call / jmp (far, absolute indirect) */
>> +        if ( x86_insn_modrm(state, NULL, &modrm_reg) != 3 ||
>> +             (modrm_reg != 3 && modrm_reg != 5) )
>> +        {
>> +            /* Wrong instruction.  Throw #GP back for now. */
>> +    default:
>> +            hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> +            emul_len = 0;
>> +            break;
>> +        }
>> +        /* Fallthrough */
>> +    case 0x62: /* bound */
>> +    case 0x9a: /* call (far, absolute) */
> I'm slightly loss here, in the case of call or jmp for example, don't
> you need the instruction pointer to point to the destination of the
> call/jmp instead of the next instruction?

No, but that is by design.

Far calls provide a selector:offset pair (either imm or mem operands),
rather than a displacement within the same code segment.

Selector may be new code selector, at which point offset is important,
and execution continues at %cs:%rip.  This case isn't interesting for
us, and doesn't vmexit in the first place.

When Selector is a Task State Segment, or Task Gate selector, a task
switch occurs (subject to cpl checks, etc).

In this case, the entrypoint of the new task is stashed in the new tasks
TSS (cs and eip fields).  The offset from the original call/jmp
instruction is discarded as it isn't relevant.   (After all,
particularly on a privilege level transition task switch, you don't want
the unprivileged caller able to start executing from somewhere which
isn't the designated entrypoint.)

Just to complete the set, selector may also be a Call Gate selector,
which is far lighter weight than a fully blown task switch, and whose
entry point is part of the Call Gate descriptor itself.

>> +    case 0xca: /* ret imm16 (far) */
>> +    case 0xcb: /* ret (far) */
>> +    case 0xcc: /* int3 */
>> +    case 0xcd: /* int imm8 */
>> +    case 0xce: /* into */
>> +    case 0xcf: /* iret */
>> +    case 0xea: /* jmp (far, absolute) */
>> +    case 0xf1: /* icebp */
>> +        break;
>> +    }
>> +
>> +    x86_emulate_free_state(state);
>> +
>> +    return emul_len;
>> +}
>> +
>> +/*
>>   * Local variables:
>>   * mode: C
>>   * c-file-style: "BSD"
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index 049b800e20..ba9c24a70c 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2776,7 +2776,41 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>  
>>      case VMEXIT_TASK_SWITCH: {
>>          enum hvm_task_switch_reason reason;
>> -        int32_t errcode = -1;
>> +        int32_t errcode = -1, insn_len = -1;
> Plain int seem better for insn_len?
>
> Also I'm not sure there's a reason that errcode uses int32_t, but
> that's not introduced here anyway.

I was just using what was already here.  I'm not sure why it is int32_t
either, but this is consistent throughout the task switch infrastructure.

~Andrew

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