[Xen-devel] Re: [PATCH] tools/ioemu: Fixing Security Hole in Qemu MSIX table access management

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 12.07.11 at 11:30, Haitao Shan <maillists.shan@xxxxxxxxx> wrote:
> I am not aware of any context of larger scope of MSI-X cleaning ups if
> you are planning to do so. As a result, I might be missing some
> important points. So please just go ahead and submit your patches.

No, I didn't have any plans besides the dealing with the proper
determination of virtual functions' MSI-X table and PBA addresses.

> 2011/7/12 Jan Beulich <JBeulich@xxxxxxxxxx>:
>>>>> On 12.07.11 at 07:24, Haitao Shan <maillists.shan@xxxxxxxxx> wrote:
>>> Hi,
>>>
>>> As reported by Jan, current Qemu does not handle MSIX table mapping
>>> properly.
>>>
>>> Details:
>>>
>>> MSI-X table resides in one of the physical BARs. When Qemu handles
>>> guest's changes to BAR register (within which, MSI-X table resides),
>>> Qemu first allows access of the whole BAR MMIO ranges and then removes
>>> those of MSI-X. There is a small window here. It is possible that on a
>>> SMP guests one vcpu could have access to the physical MSI-X
>>> configurations when another vcpu is writing BAR registers.
>>>
>>> The patch fixes this issue by first producing the valid MMIO ranges by
>>> removing MSI-X table's range from the whole BAR mmio range and later
>>> passing these ranges to Xen.
>>
>> That's only half of it - something similar would need to be done for the
>> pending bit array.
> Please justify why this read-only PBA should also be removed from
> guest access.

Because it being stated to be read-only accessible only doesn't mean
that all devices implement only read accesses (and discard writes).

> Note that we actually mask physical MSI via MSI-X table
> when guests mask it via virtualized MSI-X table.

As long as this happens from Xen, that's fine, but see below.

>> Further I'm having the impression that while you avoid assigning the
>> questionable MMIO range to the guest (which isn't a security concern
>> as long as the BAR determination for the device in the hypervisor is
>> correct), your patch doesn't prevent qemu actually mapping these
>> ranges writably and allow pci_msix_writel() to access it (which is the
>> actual open security problem).
> I totally disagree. I think Dom0 together with its management SW
> entities such as Qemu and libxc/libxl are to be trusted. It can be
> arguable to what extent Xen can trust them.

It's not a matter of trust, but one of correctness.

> Mapping that to Qemu is mainly for writing MASK bit to physical MSI-X
> table directly. Handling guests' masking MSI is already too long a
> code path.

Qemu getting this wrong (e.g. doing an unmask when the guest
requests so, but Xen wants that interrupt to be masked) can
confuse Xen significantly, up to the point where the other
interrupts (and hence the whole system) can be affected.

> If Qemu is not trusted, I would say perhaps you can move MSI
> virtualization part from Qemu to Xen itself.
> 
>>
>> Further, I don't think it's correct to remove guest access to either of
>> the two ranges altogether - either qemu needs to emulate access to
>> these, or the guest ought to be able to access the ranges directly,
>> but read-only.
> PBA is exposed to guests, unless it happens to be located on the same
> page of MSI-X table (in this case, it have to be removed), per my
> understanding.

Besides above described reason for not exposing the PBA writably,
having to handle two cases (PBA exposed and PBA invisible) would
just needlessly complicate the code. So making it consistent with
the MSI-X table is going to be both more secure and simpler to
implement.

> MSI-X table cannot be exposed to guests even read-only.

Why not? The guest (or qemu on its behalf) reading the table
doesn't do any harm. And with there being 31 currently
undefined bits in each entry, future extensions could be severely
restricted from using in guests if we're too restrictive here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel