Re: [PATCH] x86: refine guest_mode()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/04/2020 09:03, Jan Beulich wrote:
> The 2nd of the assertions as well as the macro's return value have been
> assuming we're on the primary stack. While for most IST exceptions we
> eventually switch back to the main one,

"we switch to the main one when interrupting user mode".

"eventually" isn't accurate as it is before we enter C.

>  for #DF we intentionally never
> do, and hence a #DF actually triggering on a user mode insn (which then
> is still a Xen bug) would in turn trigger this assertion, rather than
> cleanly logging state.
>
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> While we could go further and also assert we're on the correct IST
> stack in an "else" ti the "if()" added, I'm not fully convinced this
> would be generally helpful. I'll be happy to adjust accordingly if
> others think differently; at such a point though I think this should
> then no longer be a macro.
>
> --- a/xen/include/asm-x86/regs.h
> +++ b/xen/include/asm-x86/regs.h
> @@ -10,9 +10,10 @@
>      /* Frame pointer must point into current CPU stack. */                   
>  \
>      ASSERT(diff < STACK_SIZE);                                               
>  \
>      /* If not a guest frame, it must be a hypervisor frame. */               
>  \
> -    ASSERT((diff == 0) || (r->cs == __HYPERVISOR_CS));                       
>  \
> +    if ( diff < PRIMARY_STACK_SIZE )                                         
>  \
> +        ASSERT(!diff || ((r)->cs == __HYPERVISOR_CS));                       
>  \
>      /* Return TRUE if it's a guest frame. */                                 
>  \
> -    (diff == 0);                                                             
>  \
> +    !diff || ((r)->cs != __HYPERVISOR_CS);                                   
>  \

The (diff == 0) already worried me before because it doesn't fail safe,
but this makes things more problematic.  Consider the case back when we
had __HYPERVISOR_CS32.

Guest mode is strictly "(r)->cs & 3".

Everything else is expectations about how things ought to be laid out,
but for safety in release builds, the final judgement should not depend
on the expectations evaluating true.

~Andrew