Re: [PATCH v4 05/21] IOMMU/x86: restrict IO-APIC mappings for PV Dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2022 14:01, Roger Pau Monné wrote:
> On Wed, May 04, 2022 at 12:51:25PM +0200, Jan Beulich wrote:
>> On 04.05.2022 12:30, Roger Pau Monné wrote:
>>> Right, ->iomem_caps is indeed too wide for our purpose.  What
>>> about using something like:
>>>
>>> else if ( is_pv_domain(d) )
>>> {
>>>     if ( !iomem_access_permitted(d, pfn, pfn) )
>>>         return 0;
>>
>> We can't return 0 here (as RAM pages also make it here when
>> !iommu_hwdom_strict), so I can at best take this as a vague outline
>> of what you really mean. And I don't want to rely on RAM pages being
>> (imo wrongly) represented by set bits in Dom0's iomem_caps.
> 
> Well, yes, my suggestion was taking into account that ->iomem_caps for
> dom0 has mostly holes for things that shouldn't be mapped, but
> otherwise contains everything else as allowed (including RAM).
> 
> We could instead do:
> 
> else if ( is_pv_domain(d) && type != RAM_TYPE_CONVENTIONAL )
> {
>     ...
> 
> So that we don't rely on RAM being 'allowed' in ->iomem_caps?

This would feel to me like excess special casing.

>>>     if ( rangeset_contains_singleton(mmio_ro_ranges, pfn) )
>>>         return IOMMUF_readable;
>>> }
>>>
>>> That would get us a bit closer to allowed CPU side mappings, and we
>>> don't need to special case IO-APIC or HPET addresses as those are
>>> already added to ->iomem_caps or mmio_ro_ranges respectively by
>>> dom0_setup_permissions().
>>
>> This won't fit in a region of code framed by a (split) comment
>> saying "Check that it doesn't overlap with ...". Hence if anything
>> I could put something like this further down. Yet even then the
>> question remains what to do with ranges which pass
>> iomem_access_permitted() but
>> - aren't really MMIO,
>> - are inside MMCFG,
>> - are otherwise special.
>>
>> Or did you perhaps mean to suggest something like
>>
>> else if ( is_pv_domain(d) && iomem_access_permitted(d, pfn, pfn) &&
>>           rangeset_contains_singleton(mmio_ro_ranges, pfn) )
>>     return IOMMUF_readable;
> 
> I don't think this would be fully correct, as we would still allow
> mappings of IO-APIC pages explicitly banned in ->iomem_caps by not
> handling those?

CPU side mappings don't deal with the IO-APICs specifically. They only
care about iomem_caps and mmio_ro_ranges. Hence explicitly banned
IO-APIC pages cannot be mapped there either. (Of course we only do
such banning if IO-APIC pages weren't possible to represent in
mmio_ro_ranges, which should effectively be never.)

>> ? Then there would only remain the question of whether mapping r/o
>> MMCFG pages is okay (I don't think it is), but that could then be
>> special-cased similar to what's done further down for vPCI (by not
>> returning in the "else if", but merely updating "perms").
> 
> Well part of the point of this is to make CPU and Device mappings
> more similar.  I don't think devices have any business in poking at
> the MMCFG range, so it's fine to explicitly ban that range.  But I
> would have also said the same for IO-APIC pages, so I'm unsure why are
> IO-APIC pages fine to be mapped RO, but not the MMCFG range.

I wouldn't have wanted to allow r/o mappings of the IO-APICs, but
Linux plus the ACPI tables of certain vendors require us to permit
this. If we didn't, Dom0 would crash there during boot.

Jan