Re: [Qemu-devel] [PATCH 17/21] introduce memory_region_get_address() and use it in kvm/ioapic

[Igor Mammedov]

On Sat, 27 Apr 2013 20:57:26 +0000
Blue Swirl <address@hidden> wrote:

> On Sat, Apr 27, 2013 at 12:12 PM, Paolo Bonzini <address@hidden> wrote:
> > Il 27/04/2013 12:09, Blue Swirl ha scritto:
> >> On Fri, Apr 26, 2013 at 10:13 PM, Paolo Bonzini <address@hidden> wrote:
> >>> Il 26/04/2013 19:46, Igor Mammedov ha scritto:
> >>>>>> But as the address can't be changed (yet), the entire patch could be 
> >>>>>> simply:
> >>>>>> -    kioapic->base_address = s->busdev.mmio[0].addr;
> >>>>>> +    kioapic->base_address = IO_APIC_DEFAULT_ADDRESS;
> >>>> It's a bit fragile, but that for sure simpler and can work.
> >>>>
> >>>> Jan, Paolo,
> >>>> Are you ok with this approach?
> >>>>
> >>>
> >>> I think extending memory_region_find is a good idea anyway, and at this
> >>> point I don't see a reason to do the above change...
> >>
> >> The reasoning was in the part that Igor cut off:
> >>
> >> "Later, when it's possible to change the address via PIIX3 registers,
> >> we can adjust the base and pass that properly to kioapic and on to
> >> KVM.
> >>
> >> Resolving the base address every time when kvm_ioapic_put() is called
> >> is also less efficient, assuming of course that the base address
> >> changes less often than the KVM ioctl is used."
> >>
> >> I think the patch is a bit flawed. If the guest maps something else on
> >> top of IOAPIC, like LAPIC (which should be in CPU specific address
> >> spaces, but for now it lives in the global system memory space), the
> >> guest could trigger the abort() by resetting the system.
> >
> > The questions are, in order of importance:
> >
> > (1) what privileges would this require in the guest?  Answer: a lot.
> >
> > (2) is this likely to happen by chance?  Answer: no, not at all.
> >
> > (3) is there a workaround?  Answer: yes, disable in-kernel irqchip.
> 
> These questions ask if there is a risk of benevolent guests performing
> these activities and I agree that the chances are close to zero.
> 
> But the interesting question is to ask if a malevolent guest can bring
> down a VM uncontrollably this way and I think it only needs a few
> elevated privileges in a guest to do this.
> 
> The fix is to avoid abort(), which is a separate issue to whether the
> address base should be resolved for each KVM ioctl or not.
> 
> >
> > Simply setting IO_APIC_DEFAULT_ADDRESS is also flawed in my opinion.
> > I'm not sure the in-kernel irqchip handles correctly an overlap between
> > the IOAPIC and LAPIC regions, maybe an abort is predictable after all.
> 
> At least the guest needs to be stopped. Perhaps we should have a
> common function which does this and logs the guest error so we can
> start replacing calls to abort() with it.
> 
> >
> > Paolo
> 

It looks like discussion got deviated from what patch does to another issue.

this patch doesn't address/change the way how/when base_address should be
set/updated but it has it's benefits as well:
 - removes/cleanups access to private field of parent, which allows to convert
   it to non SysBusDevice
 - extended memory_region_find() opens venue for cleaning-up/re-factoring
   devices that use framebuffer which are forced currently to access
   system_address_space directly.
 

-- 
Regards,
  Igor