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

[Jan Beulich <jbeulich@suse.com>]

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? 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.

>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -958,9 +958,13 @@ guest_physmap_add_entry(struct domain *d
>>          if ( p2m_is_special(ot) )
>>          {
>>              /* Don't permit unmapping grant/foreign/direct-MMIO this way. */
>> -            domain_crash(d);
>>              p2m_unlock(p2m);
>> -            
>> +            printk(XENLOG_G_ERR
>> +                   "%pd: GFN %lx (%lx:%u:%u) -> (%lx:%u:%u) not permitted\n",
> 
> type and access need to be rendered in hex, or you need to use 0x
> prefixes to distinguish the two bases.

Will use %#lx then.

> Also, use commas rather than colons.  Visually, this is ambiguous with
> PCI BDFs, and commas match tuple notation in most programming languages
> which is the construct you're trying to represent.
> 
> Same below.

Sure, will do.

>> @@ -1302,9 +1306,50 @@ static int set_typed_p2m_entry(struct do
>>      }
>>      if ( p2m_is_special(ot) )
>>      {
>> -        gfn_unlock(p2m, gfn, order);
>> -        domain_crash(d);
>> -        return -EPERM;
>> +        bool done = false, bad = true;
>> +
>> +        /* Special-case (almost) identical mappings. */
>> +        if ( mfn_eq(mfn, omfn) && gfn_p2mt == ot )
>> +        {
>> +            /*
>> +             * For MMIO allow X to differ in the requests (to cover for
>> +             * set_identity_p2m_entry() and set_mmio_p2m_entry() differing in
>> +             * the way they specify "access"). For the hardware domain put (or
>> +             * leave) in place the more permissive of the two possibilities,
>> +             * while for DomU-s go with the more restrictive variant.
> 
> This comment needs to identify clearly that it is a temporary bodge
> intended to be removed.

Okay.

Jan