On Sat, Apr 27, 2013 at 12:12 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > Il 27/04/2013 12:09, Blue Swirl ha scritto: >> On Fri, Apr 26, 2013 at 10:13 PM, Paolo Bonzini <pbonz...@redhat.com> 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