Re: [Xen-devel] [PATCH for-4.12] x86/svm: Fix handling of ICEBP intercepts

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.02.19 at 20:44, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/02/2019 09:16, Jan Beulich wrote:
>>>>> On 01.02.19 at 18:09, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 01/02/2019 16:55, Jan Beulich wrote:
>>>>>>> On 01.02.19 at 17:25, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> If it were just getting insn_len incorrectly as 0, then the guest would
>>>>> livelock as we wouldn't inject the #DB with trap semantics it requires,
>>>> I'm confused again: Why trap semantics? The ICEBP has fault
>>>> semantics as you confirmed above.
>>> The ICEBP intercept has fault semantics.  An ICEBP instruction executing
>>> in the guest has trap semantics.
>> Oh, okay - I was mis-remembering this aspect.
>>
>>>>> but as the #GP is already raised, this will combine to #DF.
>>>> How that? #DB is a benign exception, so according to the table on the
>>>> #DF page in the SDM, with #GP it shouldn't combine to #DF.
>>> #GP is raised first.  It is contributory.
>>>
>>> A subsequent #DB getting raised causes #GP to turn into #DF.
>> That's based on what?
> 
> Based on actually trying this error scenario.
> 
> (d1) --- Xen Test Framework ---
> (d1) Environment: HVM 64bit (Long mode 4 levels)
> (d1) Hello World
> (XEN) ** Got ICEBP intercept
> (d1) ******************************
> (d1) PANIC: Unhandled exception at 0046:0000000000000008
> (d1) Vec 8 #DF[4740]
> (d1) ******************************
> 
> Clearly something is off-by-one in the eventual stack frame, which
> probably means we've got a a bug in svm_inject_event().  I suspect the
> escalation to #DF doesn't overwrite the #DB's "no error code"
> information, but I've not investigated yet.

I think this is because svm_inject_event(), when calling
hvm_combine_hw_exceptions(), legitimately assumes the new
event can only possibly be a HW_EXCEPTION. It therefore
doesn't overwrite _event.type, which in this (bogus) case is
PRI_SW_EXCEPTION. As a result eventinj.fields.ev won't get set,
as we won't reach the subsequent switch()'s default case.

I don't think that's worth fixing though, as the assumption is
correct (afaict). If anything we could add a respective ASSERT(),
but of course only once the bad call is gone.

Jan



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