Re: [Xen-devel] [PATCH v2 3/5] x86/pv: Introduce pv_create_exception_frame()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.02.18 at 15:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> v2:
>  * Use domain_crash() rather than domain_crash_sync().  All callers
>    immediately continue to {compat_}test_all_events
>  * Count the number of frame[] entries correctly
>  * Consistently use 64bit operations when adjusting the root frame
>  * Introduce a compat_addr_ok() check for the 32bit side.  The ASM version
>    didn't have protection attempting to write into the compat p2m, other than
>    hitting a #PF while trying.

I'm not sure I see the value of the extra check - we've got to handle
#PF anyway. But I also won't insist on dropping it again.

> +void pv_create_exception_frame(void)
> +{
> +    struct vcpu *curr = current;
> +    struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    const bool user_mode_frame = !guest_kernel_mode(curr, regs);
> +    uint8_t *evt_mask = &vcpu_info(curr, evtchn_upcall_mask);
> +    unsigned int flags, bytes, missing;
> +
> +    ASSERT_NOT_IN_ATOMIC();
> +
> +    if ( unlikely(null_trap_bounce(curr, tb)) )
> +    {
> +        gprintk(XENLOG_ERR, "Fatal: Attempting to inject null trap 
> bounce\n");
> +        domain_crash(curr->domain);
> +        return;
> +    }
> +
> +    /* Fold the upcall mask and architectural IOPL into the guests rflags. */
> +    flags  = regs->rflags & ~(X86_EFLAGS_IF | X86_EFLAGS_IOPL);

regs->eflags would be more consistent with the type of flags.

> +    flags |= ((*evt_mask ? 0 : X86_EFLAGS_IF) |
> +              (VM_ASSIST(curr->domain, architectural_iopl)
> +               ? curr->arch.pv_vcpu.iopl : 0));
> +
> +    if ( is_pv_32bit_vcpu(curr) )
> +    {
> +        /* { [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
> +        unsigned int frame[6], *ptr = frame, ksp =
> +            (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->esp);
> +
> +        if ( tb->flags & TBF_EXCEPTION_ERRCODE )
> +            *ptr++ = tb->error_code;
> +
> +        *ptr++ = regs->eip;
> +        *ptr++ = regs->cs | ((unsigned int)*evt_mask << 16);
> +        *ptr++ = flags;
> +
> +        if ( user_mode_frame )
> +        {
> +            *ptr++ = regs->esp;
> +            *ptr++ = regs->ss;
> +        }
> +
> +        /* Copy the constructed frame to the guest kernel stack. */
> +        bytes = _p(ptr) - _p(frame);
> +        ksp -= bytes;
> +
> +        if ( unlikely(!__compat_access_ok(curr->domain, ksp, bytes)) )
> +        {
> +            gprintk(XENLOG_ERR, "Fatal: Bad guest kernel stack %p\n", 
> _p(ksp));

While I understand that you don't want to deal with non-flat SS here
(yet), I think it would be prudent to log %ss nevertheless.

> +            domain_crash(curr->domain);
> +            return;
> +        }
> +
> +        if ( unlikely((missing = __copy_to_user(_p(ksp), frame, bytes)) != 
> 0) )
> +        {
> +            gprintk(XENLOG_ERR, "Fatal: Fault while writing exception 
> frame\n");
> +            show_page_walk(ksp + missing);

"missing" is the right name, but the use is wrong - ITYM
"ksp + bytes - missing" (same on the 64-bit path then).

If you agree with (and have carried out) the suggested changes
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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