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: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?

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

> >>>> ? 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 ...

For HVM/PVH I think we want o be consistent as long as it's doable (we
can't provide devices access to the emulated MMCFG there for example).

For PV I guess it's also a worthy goal if it makes the code easier.
PV (and PV dom0 specially) is already a very custom platform with
weird properties (like the mapping of the IO-APIC and HPET regions RO
or no mappings at all).

Thanks, Roger.