Re: [PATCH 03/16] x86/traps: Factor out exception_fixup() and make printing consistent

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/05/2020 14:20, Jan Beulich wrote:
> On 02.05.2020 00:58, Andrew Cooper wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -774,10 +774,27 @@ static void do_reserved_trap(struct cpu_user_regs 
>> *regs)
>>            trapnr, vec_name(trapnr), regs->error_code);
>>  }
>>  
>> +static bool exception_fixup(struct cpu_user_regs *regs, bool print)
>> +{
>> +    unsigned long fixup = search_exception_table(regs);
>> +
>> +    if ( unlikely(fixup == 0) )
>> +        return false;
>> +
>> +    /* Can currently be triggered by guests.  Make sure we ratelimit. */
>> +    if ( IS_ENABLED(CONFIG_DEBUG) && print )
> I didn't think we consider dprintk()-s a possible security issue.
> Why would we consider so a printk() hidden behind
> IS_ENABLED(CONFIG_DEBUG)? IOW I think one of XENLOG_GUEST and
> IS_ENABLED(CONFIG_DEBUG) wants dropping.

Who said anything about a security issue?

I'm deliberately not using dprintk() for the reasons explained in the
commit message, so IS_ENABLED(CONFIG_DEBUG) is to cover that.

XENLOG_GUEST is because everything (other than the boot path) hitting
this caused directly by a guest action, and needs rate-limiting.  This
was not consistent before.

>
>> @@ -1466,12 +1468,11 @@ void do_page_fault(struct cpu_user_regs *regs)
>>          if ( pf_type != real_fault )
>>              return;
>>  
>> -        if ( likely((fixup = search_exception_table(regs)) != 0) )
>> +        if ( likely(exception_fixup(regs, false)) )
>>          {
>>              perfc_incr(copy_user_faults);
>>              if ( unlikely(regs->error_code & PFEC_reserved_bit) )
>>                  reserved_bit_page_fault(addr, regs);
>> -            regs->rip = fixup;
> I'm afraid this modification can't validly be pulled ahead -
> show_execution_state(), as called from reserved_bit_page_fault(),
> wants to / should print the original RIP, not the fixed up one.

This path is bogus to begin with.  Any RSVD pagefault (other than the
Shadow MMIO fastpath, handled earlier) is a bug in Xen and should be
fatal rather than just a warning on extable-tagged instructions.

Amongst other things, it would consistent an L1TF-vulnerable gadget. 
The MMIO fastpath is only safe-ish because it also inverts the upper
address bits.

~Andrew