On Sat, 27 Apr 2013 20:57:26 +0000 Blue Swirl <blauwir...@gmail.com> wrote:
> 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 > 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
