Re: [Xen-devel] [V9 PATCH 7/8] pvh dom0: check for vioapic null ptr in vioapic_range

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Thu, 24 Apr 2014 07:49:44 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 23.04.14 at 23:18, <mukesh.rathor@xxxxxxxxxx> wrote:
> > On Wed, 23 Apr 2014 10:07:25 +0100
> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> > 
> >> >>> On 23.04.14 at 02:11, <mukesh.rathor@xxxxxxxxxx> wrote:
> >> > On Tue, 22 Apr 2014 08:33:29 +0100
> >> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >> >> >>> On 22.04.14 at 02:59, <mukesh.rathor@xxxxxxxxxx> wrote:
> > ......
.....
> >> >   So it must have been the third one that I had observed the
> >> >   vioapic_range crash in a while ago, and had made note of it.
> >> > Looking at it:
> >> > 
> >> >     if ( (p2mt == p2m_mmio_dm) ||
> >> >          (access_w && (p2mt == p2m_ram_ro)) )
> >> >     {
> >> >         put_gfn(p2m->domain, gfn);
> >> >         if ( !handle_mmio() )
> >> > 
> >> > doesn't seem apply to domu. Unfortunately, I can't reproduce it
> >> > now so maybe it was an ept violation due to some bug, and a
> >> > crash in vioapic_range before printing the gfn/mfns etc by
> >> > ept_handle_violation made me make a note to put a check in it.
> >> 
> >> Which makes me think that we don't need the patch at all.
> > 
> > Well, without this patch, in case of dom0 EPT violation, dom0 will
> > not die gracefully printing gfn/mfn/etc.. info. But instead it will
> > show fault in vioapic_range. 
> > 
> > 
> > ept_handle_violation() 
> >        hvm_hap_nested_page_fault()
> >              -> handle_mmio() -----> vioapic_range() : KABOOM!!
> > 
> >        gdprintk(XENLOG_ERR, "EPT violation %#lx (%c%c%c/%c%c%c), "
> >                     "gpa %#"PRIpaddr", mfn %#lx, type %i.\n",
> >                                  qualification,  <=== NOT REACHED
> >           .......

... 

> So we're moving in circles I'm afraid: You told us that I/O emulation
> is being handled by a separate path from HVM's, which means either
> handle_mmio() separates the cases itself, or doesn't even get
> called, only to then again show us the call sequence above. One
> of the two statements can't be correct, and what I'd like you to do
> about it depends on which one it is.

Ok, nailed it!! The issue is guest causing EPT violation because
of bad pfn in it's pte, not for mmio emulation.

ept_handle_violation calls hvm_hap_nested_page_fault() which has
three calls to handle_mmio(). The first two are skipped for pvh,
but before the 3rd call, the function does:

    mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
                              P2M_ALLOC | (access_w ? P2M_UNSHARE : 0), NULL);

Since, the pfn is invalid, this returns p2m_mmio_dm which is the
default return for non-ram for, I think, historical reasons: 

    ept_get_entry():
          *t = p2m_mmio_dm;

This causes 3rd handle_mmio in hvm_hap_nested_page_fault to be called for pvh:

    if ( (p2mt == p2m_mmio_dm) ||
         (access_w && (p2mt == p2m_ram_ro)) )
    {
        put_gfn(p2m->domain, gfn);
        if ( !handle_mmio() )  <==========
            hvm_inject_hw_exception(TRAP_gp_fault, 0);

which would result in vioapic_range panic in xen.

I was on the right track before, just couldn't come up with where 
p2m_mmio_dm was coming from. Anyways, so:


 1. We can go with the patch that adds pvh_mmio_handlers in anticipation 
    of future msix, hpet support:

+static const struct hvm_mmio_handler *const
+pvh_mmio_handlers[HVM_MMIO_HANDLER_NR] =
...

    That would naturally return X86EMUL_UNHANDLEABLE in this case. 

    OUTCOME: guest would be injected with GPF, but xen won't crash it.


 2. We could go with vioapic null ptr check in vioapic_range() itself,
    thus causing handle_mmio to return X86EMUL_UNHANDLEABLE also.

    OUTCOME: guest would be injected with GPF, but xen won't crash it.


 3. Add pvh check above in hvm_hap_nested_page_fault:

        put_gfn(p2m->domain, gfn);
        if ( is_pvh_vcpu(v) )
        {
            rc = 0;
            goto out;
        }
        if ( !handle_mmio() )  <==========
            hvm_inject_hw_exception(TRAP_gp_fault, 0);
...

    OUTCOME: xen would crash guest with the nice ept violation message.


 4. Add check for pvh in handle_mmio:

 int handle_mmio(void)
 {
      int rc;

      if ( is_pvh_vcpu(current) )
          return X86EMUL_UNHANDLEABLE;
     ... 

    OUTCOME: guest would be injected with GPF, but xen won't crash it.


 5. Do 1/2/4 above, but in addition, in hvm_hap_nested_page_fault() do:

    if ( (p2mt == p2m_mmio_dm) ||
         (access_w && (p2mt == p2m_ram_ro)) )
    {
        put_gfn(p2m->domain, gfn);
        rc = 1;
        if ( !handle_mmio() )  <==========
        {
            if ( is_pvh_vcpu )
                rc = 0;
            else
                hvm_inject_hw_exception(TRAP_gp_fault, 0);
        }
        goto out;
    }

    OUTCOME: xen would crash guest with the nice ept violation message
             that prints all the details.

Please lmk your thoughts and preference, and I'll submit patch. The patch
would not be part of dom0 pvh series, as a pvh domU ept violation
could cause this xen panic too in vioapic_range. We may wanna backport
it too (not sure because of experimental nature of the feature).

thanks,
Mukesh

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