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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 04, 2022 at 03:55:09PM +0200, Jan Beulich wrote:
> 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).

My idea was for whatever check we add for PV to also cover HPET, which
is in a similar situation (can be either blocked in ->iomem_caps or in
mmio_ro_ranges).

Thanks, Roger.