On 01.09.2021 14:47, Andrew Cooper wrote: > On 31/08/2021 16:38, 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. > > But you're conflating a feature (under question anyway, because I gave > you an example where I expect this will collide in a regular domU > already),
I don't think your example fits: hvmloader moving RAM will first convert the p2m slot to non-present. Then a ROM page can get mapped there quite fine. A direct transition (without going through n/p) would not work independent of the change here: The MFNs would differ, as would the p2m types. > with an emergency bugfix to unbreak staging caused by an > unexpected interaction in a security hotfix. > > At an absolute minimum, this patch needs splitting in two to separate > the bugfix from the proposed feature. Well, okay, I will split the patch, despite not being convinced this will do us any good - we'd backport just the part you consider a bug fix, but not the part you deem a feature (and which I consider part of the bug fix). >>>> 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. Actually I think I was missing an important aspect here: The code in question gets used not only for PVH, but also for HVM, where pass- through is a thing. Hence I'll restrict the "feature" part to Dom0 for now. >>> 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? > > Yes. > >> Because that's >> what's getting the way here. > > On a real machine, you really can write some executable code into an > E820 reserved region and jump to it. You can also execute code from > real BARs is you happen to know that they are prefetchable (or you're a > glutton for UC reads...) > > And there is the WPBT ACPI table which exists specifically to let > firmware inject drivers/applications into a windows environment, and may > come out of the SPI ROM in the first place. > > > Is it sensible to execute an E820 reserved region, or unmarked BAR? > Probably not. > > Should it work, because that's how real hardware behaves? Absolutely. > > Any restrictions beyond that want handling by some kind of introspection > agent which has a policy of what to do with legal-but-dodgy-looking actions. IOW you suggest we remove p2m_access_t parameters from various functions, going with just default access? Of course, as pointed out in another reply, we'll need to split p2m_mmio_direct into multiple types then, at the very least to honor the potential r/o restriction of AMD IOMMU unity mapped regions. (FAOD all of this isn't a short term plan anyway, at least afaic.) Jan
