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

[Blue Swirl]

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