On Wed, Sep 01, 2021 at 11:53:03AM +0200, Jan Beulich wrote: > On 01.09.2021 10:50, Roger Pau Monné wrote: > > On Tue, Aug 31, 2021 at 05:38:49PM +0200, Jan Beulich wrote: > >> On 31.08.2021 17:25, Andrew Cooper wrote: > >>> On 31/08/2021 14:26, Jan Beulich wrote: > >>>> On 31.08.2021 15:16, Andrew Cooper wrote: > >>>>> On 30/08/2021 14:02, Jan Beulich wrote: > >>>>>> Further permit "access" to differ in the "executable" attribute. While > >>>>>> ideally only ROM regions would get mapped with X set, getting there is > >>>>>> quite a bit of work. Therefore, as a temporary measure, permit X to > >>>>>> vary. For Dom0 the more permissive of the types will be used, while for > >>>>>> DomU it'll be the more restrictive one. > >>>>> Split behaviour between dom0 and domU based on types alone cannot > >>>>> possibly be correct. > >>>> True, but what do you do. > >>>> > >>>>> DomU's need to execute ROMs too, and this looks like will malfunction if > >>>>> a ROM ends up in the region that HVMLoader relocated RAM from. > >>>>> > >>>>> As this is a temporary bodge emergency bugfix, don't try to be clever - > >>>>> just take the latest access. > >>>> And how do we know that that's what is going to work? > >>> > >>> Because it's the pre-existing behaviour. > >> > >> Valid point. But for the DomU case there simply has not been any > >> pre-existing behavior. Hence my desire to be restrictive initially > >> there. > >> > >>>> We should > >>>> strictly accumulate for Dom0. And what we do for DomU is moot for > >>>> the moment, until PCI passthrough becomes a thing for PVH. Hence > >>>> I've opted to be restrictive there - I'd rather see things break > >>>> (and getting adjusted) when this future work actually gets carried > >>>> out, than leave things permissive for no-one to notice that it's > >>>> too permissive, leading to an XSA. > >>> > >>> Restricting execute permissions is something unique to virt. It doesn't > >>> exist in a non-virtualised system, as I and D side reads are > >>> indistinguishable outside of the core. > >>> > >>> Furthermore, it is inexpressible on some systems/configurations. > >>> > >>> Introspection is the only technology which should be restricting execute > >>> permissions in the p2m, and only when it takes responsibility for > >>> dealing with the fallout. > >> > >> IOW are you saying that the calls to set_identity_p2m_entry() > >> (pre-dating XSA-378) were wrong to use p2m_access_rw? Because that's > >> what's getting the way here. > > > > I did wonder this before, because I saw some messages on a couple of > > systems about mappings override, and I'm not sure why we need to use > > p2m_access_rw. My first thought was to suggest to switch to use the > > default access type for the domain, like set_mmio_p2m_entry does. > > > > I have to admit I'm not sure I see the point of preventing execution, > > but it's possible I'm missing something. > > Well, what good can come from allowing execution from, say, the > IO-APIC or LAPIC pages? Or other MMIO-mapped register space?
map_mmio_regions does already map BARs with execute permissions, so it's just some MMIO regions that get mapped without execution permissions, which makes all this confusing. > Insn > fetches might even trip bad hardware behavior in such a case by > being the wrong granularity. Normal reads could also trigger such bad hardware behavior, so I'm not sure preventing execution provides Xen any more safety. > It's imo really only ROM space which > ought to have execution permitted. > > The issue isn't just with execution, though, and as a result I may > need to change the logic here to also include at least W. As of > one of the XSA-378 changes we may now pass just p2m_access_r to > iommu_identity_mapping(), if the ACPI tables on an AMD system were > saying so. (We may also pass p2m_access_w, but I sincerely hope no > firmware would specify write but no read access.) > > Similarly in "IOMMU/x86: restrict IO-APIC mappings for PV Dom0" I > now pass p2m_access_r to set_identity_p2m_entry(). Not really I think, as PVH dom0 is the only user of the set_identity_p2m_entry call in arch_iommu_hwdom_init, and we should never identity map the IO-APIC range in that case because a set of emulated IO-APIC replacements are provided and those require ranges to be unmapped so that accesses can be trapped. > > I suppose an underlying issue is the mixed purpose of using > p2m_access_*, which possibly has been against the intentions in the > first place. We cannot, for example, express r/o access to an MMIO > page without using p2m_access_r (or p2m_access_rx), as there's no > suitable p2m type to express this via type alone. We may need to > split p2m_mmio_direct into multiple types (up to 7), I guess, if > we wanted to remove this (ab)use of p2m_access_*. My main complaint is mostly with the fact that some MMIO ranges are mapped without execute permissions when mapped by set_identity_p2m_entry vs map_mmio_regions that will map them with the default permissions and that has execution set. If the mappings from arch_iommu_hwdom_init for example would use the default permissions that could solve quite a lot of the issues there AFAIC. Roger.
