Re: [PATCH v2 1/4] x86/dom0: prevent access to MMCFG areas for PVH Dom0

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.08.17 at 15:54, <roger.pau@citrix.com> wrote:
> On Tue, Aug 22, 2017 at 06:26:23AM -0600, Jan Beulich wrote:
>> >>> On 11.08.17 at 18:43, <roger.pau@citrix.com> wrote:
>> > They are emulated by Xen, so they must not be mapped into Dom0 p2m.
>> > Introduce a helper function to add the MMCFG areas to the list of
>> > denied iomem regions for PVH Dom0.
>> 
>> "They are" or "They are going to be"?
> 
> This started as a series on top of vPCI, but I think it has a chance
> of getting in before vPCI. I will change it.

I guessed this would be the reason, but while reviewing the vPCI
series you've said somewhere functionality from the series here
would be implied.

>> > --- a/xen/arch/x86/dom0_build.c
>> > +++ b/xen/arch/x86/dom0_build.c
>> > @@ -440,6 +440,10 @@ int __init dom0_setup_permissions(struct domain *d)
>> >              rc |= rangeset_add_singleton(mmio_ro_ranges, mfn);
>> >      }
>> >  
>> > +    /* For PVH prevent access to the MMCFG areas. */
>> > +    if ( dom0_pvh )
>> > +        rc |= pci_mmcfg_set_domain_permissions(d);
>> 
>> What about ones reported by Dom0 later on? Which then raises the
>> question whether ...
> 
> This should be dealt with in the PHYSDEVOP_pci_mmcfg_reserved handler.
> But since you propose to do white listing, I guess it doesn't matter
> that much anymore.

Well, a fundamental question is whether white listing would work in
the first place. I could see room for severe problems e.g. with ACPI
methods wanting to access MMIO that's not described by any PCI
devices' BARs. Typically that would be regions in the chipset which
firmware is responsible for configuring/managing, the addresses of
which can be found/set in custom config space registers.

>> > @@ -175,6 +177,25 @@ void pci_mmcfg_arch_disable(unsigned int idx)
>> >             cfg->pci_segment, cfg->start_bus_number, cfg->end_bus_number);
>> >  }
>> >  
>> > +int pci_mmcfg_set_domain_permissions(struct domain *d)
>> > +{
>> > +    unsigned int idx;
>> > +    int rc = 0;
>> > +
>> > +    for ( idx = 0; idx < pci_mmcfg_config_num; idx++ )
>> > +    {
>> > +        const struct acpi_mcfg_allocation *cfg = pci_mmcfg_virt[idx].cfg;
>> > +        unsigned long start = PFN_DOWN(cfg->address) +
>> > +                              PCI_BDF(cfg->start_bus_number, 0, 0);
>> > +        unsigned long end = PFN_DOWN(cfg->address) +
>> > +                            PCI_BDF(cfg->end_bus_number, ~0, ~0);
>> > +
>> > +        rc |= iomem_deny_access(d, start, end);
>> 
>> ... this shouldn't be unnecessary by, other than PV Dom0,
>> starting out with no I/O memory being made accessible (i.e.
>> white listing just like we decided we would do for other
>> properties for PVH).
> 
> So would you like to switch to this white listing mode even for PV
> Dom0, or just for PVH?

No, I certainly don't think we should touch PV here.

> Should reserved regions and holes be added to it? Maybe only reserved
> regions?

See above - reserved regions may be a minimum that's needed to
be added, but then again we can't be certain all BIOSes properly
report everything in use by the chipset/firmware as reserved. Otoh
they're called reserved because no-one outside of the firmware
should touch them.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel