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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2022 15:46, Roger Pau Monné wrote:
> On Wed, May 04, 2022 at 03:19:16PM +0200, Jan Beulich wrote:
>> 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).
> 
> It was my understanding that when using iommu_hwdom_inclusive (or
> iommu_hwdom_reserved if the IO-APIC page is a reserved region) we
> still want to deny access to the IO-APIC page if it's not in
> iomem_caps, and the proposed conditional won't do that.
> 
> So I guess the discussion is really whether
> iommu_hwdom_{inclusive,reserved} take precedence over ->iomem_caps?

I think the intended interaction is not spelled out anywhere. I
also think that it is to be expected for such interaction to be
quirky; after all the options themselves are quirks.

> It seems a bit inconsistent IMO to enforce mmio_ro_ranges but not
> ->iomem_caps when using iommu_hwdom_{inclusive,reserved}.

In a way, yes. But as said before - it's highly theoretical for
IO-APIC pages to not be in ->iomem_caps (and this case also won't
go silently).

Jan