[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 13/02/17 11:40, Jan Beulich wrote:
>>>> 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.

When I considered this first, my plan was to catch the fault and crash
the domain, rather than allow it to continue (FPU exceptions being the
exception).

One way or another, by the time we encounter a fault in the stubs,
something has gone wrong, and crashing the domain is better than
crashing the host.  (In fact, I am looking to extend this principle
further, e.g. with vmread/vmwrite failures.)

I don't see #PF being meaningfully different to #GP or #SS here.  If we
get a fault, an action was stopped, but we can never catch the issues
which don't fault in the first place.

#UD is a little more tricky.  It either means that we created a
malformed stub, or we didn't have sufficient feature checking, both of
which are emulation bugs.  This could be passed back to the domain, but
I'd err on the side of making it more obvious by crashing the domain. 
(Perhaps changing behaviour based on debug?)

>
>>> ---
>>> 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).

We don't necessarily need to use the extable infrastructure, and you
don't appear to be using a unique key at all.

>
>>> 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?

Sorry - I was thinking it over and forgot to comment before sending. 
Your suggestion is fine, but doesn't it disappear if/when we fold the
existing fpu_exception_callback() into this more generic infrastructure.

>
>>> @@ -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.

As before, it would be better overall to result in a domain_crash() than
a host 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).

Ah - I see now, by excluding the valid stack range and falling out
without recovery.

>
>>> +    {
>>> +        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.

Ah. That certainly is awkward.

> 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.

An alternative might be have a per_cpu() variable filled in by the
exception handler.  This would avoid any need to play with the return
address and stack under the feet of the stub.

~Andrew

_______________________________________________
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®.