On 06.09.2021 21:53, Andrew Cooper wrote: > On 01/09/2021 14:08, Jan Beulich wrote: >>>>> 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? > > p2m_access_t was very obviously a bodge when introduced, and I doubt it > would pass today's review standards. > > It is definitely a mis-design, given its ill-specified and overlapping > semantics with respect to the p2m type. > >> 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.) > > I don't think that will be necessary. > > IVMDs are almost non-existent, and given how many other areas of the AMD > IOMMU spec are totally unused, I wouldn't be surprised if r/o unity > mappings were in that category too. There's no obvious usecase for r/o, > as anything critical enough in the platform to have an IVMD in the first > place will also be non-trivial enough to require bidirectional > communication somehow. > > The unity mapping only says "this device requires read-only access". It > doesn't say "this must be mapped read-only", and it is legitimate to map > a r/o unity mapping as r/w.
Well, imo that's extending what "Write permission. 1b=writeable, 0b=not writeable" and "Read permission. 1b=readable, 0b=not readable" in the spec say. "Permission" to me doesn't mean what you say. Nevertheless I would perhaps not insist (as I've already made clear I don't see a strong need to support w/o mappings), if ... > If such a case actually exists, there's clearly one agent in the system > with r/w access into the r/o range, and mapping it r/w is equivalent to > the "IOMMU not enabled in the first place" case which is the default > case for most software for the past decade-and-a-bit. > > In other words, I don't think the r/o unit maps on their own are a good > enough reasons to split the type. If we gain other reasons then fine, > but this seems like chunk of complexity with no real users. ... there wasn't already a 2nd use for this: The IO-APIC mappings (see my respective pending patch). Jan
