[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 01/11] x86emul: catch exceptions occurring in stubs



>>> On 10.02.17 at 17:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 01/02/17 11:12, Jan Beulich wrote:
>> Before adding more use of stubs cloned from decoded guest insns, guard
>> ourselves against mistakes there: Should an exception (with the
>> noteworthy exception of #PF) occur inside the stub, forward it to the
>> guest.
> 
> Why exclude #PF ? Nothing in a stub should be hitting a pagefault in the
> first place.

To be honest, I've been considering to limit this to just #UD. We
clearly shouldn't hide memory addressing issues, as them going
by silently means information leaks. Nevertheless including #PF
here would be a trivial change to the patch.

>> ---
>> There's one possible caveat here: A stub invocation immediately
>> followed by another instruction having fault revovery attached to it
>> would not work properly, as the table lookup can only ever find one of
>> the two entries. Such CALL instructions would then need to be followed
>> by a NOP for disambiguation (even if only a slim chance exists for the
>> compiler to emit things that way).
> 
> Why key on return address at all?  %rip being in the stubs should be
> good enough.

Well, we need unique (key-address, recovery-address) tuples,
and key-address can't possibly be an address inside the stub
(for both the address varying between CPUs and said uniqueness
requirement).

>> TBD: Instead of adding a 2nd search_exception_table() invocation to
>>      do_trap(), we may want to consider moving the existing one down:
>>      Xen code (except when executing stubs) shouldn't be raising #MF
>>      or #XM, and hence fixups attached to instructions shouldn't care
>>      about getting invoked for those. With that, doing the HVM special
>>      case for them before running search_exception_table() would be
>>      fine.

No opinion at all on this aspect?

>> @@ -85,15 +86,88 @@ search_one_extable(const struct exceptio
>>  }
>>  
>>  unsigned long
>> -search_exception_table(unsigned long addr)
>> +search_exception_table(const struct cpu_user_regs *regs, bool check_stub)
>>  {
>> -    const struct virtual_region *region = find_text_region(addr);
>> +    const struct virtual_region *region = find_text_region(regs->rip);
>> +    unsigned long stub = this_cpu(stubs.addr);
>>  
>>      if ( region && region->ex )
>> -        return search_one_extable(region->ex, region->ex_end - 1, addr);
>> +        return search_one_extable(region->ex, region->ex_end - 1, 
>> regs->rip);
>> +
>> +    if ( check_stub &&
>> +         regs->rip >= stub + STUB_BUF_SIZE / 2 &&
>> +         regs->rip < stub + STUB_BUF_SIZE &&
>> +         regs->rsp > (unsigned long)&check_stub &&
>> +         regs->rsp < (unsigned long)get_cpu_info() )
> 
> How much do we care about accidentally clobbering %rsp in a stub?

I think we can't guard against everything, but we should do the
best we reasonably can. I.e. in the case here, rather than
reading the return (key) address from somewhere outside the
stack (easing a possible attacker's job), don't handle the fault
at all, and instead accept the crash.

> If we encounter a fault with %rip in the stubs, we should terminate
> obviously if %rsp it outside of the main stack.  Nothing good can come
> from continuing.

This is what above code guarantees (or is at least meant to
guarantee).

>> +    {
>> +        unsigned long retptr = *(unsigned long *)regs->rsp;
>> +
>> +        region = find_text_region(retptr);
>> +        retptr = region && region->ex
>> +                 ? search_one_extable(region->ex, region->ex_end - 1, 
>> retptr)
>> +                 : 0;
>> +        if ( retptr )
>> +        {
>> +            /*
>> +             * Put trap number and error code on the stack (in place of the
>> +             * original return address) for recovery code to pick up.
>> +             */
>> +            *(unsigned long *)regs->rsp = regs->error_code |
>> +                ((uint64_t)(uint8_t)regs->entry_vector << 32);
>> +            return retptr;
> 
> I have found an alternative which has proved very neat in XTF.
> 
> By calling the stub like this:
> 
> asm volatile ("call *%[stub]" : "=a" (exn) : "a" (0));
> 
> and having this fixup write straight into %rax, the stub ends up
> behaving as having an unsigned long return value.  This avoids the need
> for any out-of-line code recovering the exception information and
> redirecting back as if the call had completed normally.

My main reservation against this is that some instructions use
rAX as a fixed operand ({,V}PCMPESTR{I,M}) for example. Plus
what would you intend to do with the RIP to return to and/or
the extra item on the stack? I'd like the generic mechanism here
to impose as little restrictions on its potential use cases as
possible, just like the pre-existing mechanism does. The fiddling
with stack and return address is already more than I really feel
happy with, but I can't see a way to do without.

> One subtle trap I fell over is you also need a valid bit to help
> distinguish #DE, which always has an error code of 0.

Hence I prefer to not use 0 as the initializer, but ~0.

>> --- a/xen/include/asm-x86/uaccess.h
>> +++ b/xen/include/asm-x86/uaccess.h
>> @@ -275,7 +275,16 @@ extern struct exception_table_entry __st
>>  extern struct exception_table_entry __start___pre_ex_table[];
>>  extern struct exception_table_entry __stop___pre_ex_table[];
>>  
>> -extern unsigned long search_exception_table(unsigned long);
>> +union stub_exception_token {
>> +    struct {
>> +        uint32_t ec;
> 
> ec only needs to be 16 bits wide, which very helpfully lets it fit into
> an unsigned long, even for 32bit builds, and 8 bits at the top for extra
> metadata.

We don't need to care about 32-bit builds in the hypervisor anymore,
and acting on 32-bit quantities is cheaper. If we needed the bits for
something else, we could of course shrink the field.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.