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

[Jan Beulich <jbeulich@xxxxxxxx>]

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