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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2022 15:00, Roger Pau Monné wrote:
> On Wed, May 04, 2022 at 02:12:58PM +0200, Jan Beulich wrote:
>> 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.
> 
> What about placing this in the 'default:' label on the type switch a
> bit above?

I'd really like to stick to the present layout of where the special
casing is done, with PV and PVH logic at least next to each other. I
continue to think the construct I suggested (still visible below)
would do.

>>>>>     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.)
> 
> I think I haven't expressed myself correctly.
> 
> This construct won't return 0 for pfns not in iomem_caps, and hence
> could allow mapping of addresses not in iomem_caps?

I'm afraid I don't understand: There's an iomem_access_permitted()
in the conditional. How would this allow mapping pages outside of
iomem_caps? The default case higher up has already forced perms to
zero for any non-RAM page (unless iommu_hwdom_inclusive).

>>>> ? 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.
> 
> Right, but those are required for the CPU only.  I think it's a fine
> goal to try to have similar mappings for CPU and Devices, and then
> that would also cover MMCFG in the PV case.  Or else it fine to assume
> CPU vs Device mappings will be slightly different, and then don't add
> any mappings for IO-APIC, HPET or MMCFG to the Device page tables
> (likely there's more that could be added here).

It being different is what Andrew looks to strongly dislike. And I agree
with this up to a certain point, i.e. I'm having a hard time seeing why
we should put in MMCFG mappings just for this reason. But if consensus
was that consistency across all types of MMIO is the goal, then I could
live with also making MMCFG mappings ...

Jan