Re: [PATCH 2/4] x86/P2M: relax guarding of MMIO entries

["Roger Pau Monné" <roger.pau@citrix.com>]

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.

Thanks, Roger.