Re: [PATCH v3 9/9] x86/P2M: relax permissions of PVH Dom0's MMIO entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2021 13:10, Roger Pau Monné wrote:
> On Tue, Sep 21, 2021 at 09:21:11AM +0200, Jan Beulich wrote:
>> To become independent of the sequence of mapping operations, permit
>> "access" to accumulate for Dom0, noting that there's not going to be an
>> introspection agent for it which this might interfere with. While e.g.
>> ideally only ROM regions would get mapped with X set, getting there is
>> quite a bit of work. Plus the use of p2m_access_* here is abusive in the
>> first place.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v3: Move last in series, for being controversial.
>> v2: Split off from original patch. Accumulate all of R, W, and X.
>>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1319,6 +1319,18 @@ static int set_typed_p2m_entry(struct do
>>              return -EPERM;
>>          }
>>  
>> +        /*
>> +         * Gross bodge, to go away again rather sooner than later:
>> +         *
>> +         * For MMIO allow access permissions to accumulate, but only for 
>> Dom0.
>> +         * Since set_identity_p2m_entry() and set_mmio_p2m_entry() differ in
>> +         * the way they specify "access", this will allow the ultimate 
>> result
>> +         * to be independent of the sequence of operations.
> 
> Wouldn't it be better to 'fix' those operations so that they can work
> together?

Yes, but then we should do this properly by removing all abuse of
p2m_access_t.

> It's my understanding that set_identity_p2m_entry is the one that has
> strong requirements regarding the access permissions, as on AMD ACPI
> tables can specify how should regions be mapped.
> 
> A possible solution might be to make set_mmio_p2m_entry more tolerant
> to how present mappings are handled. For once that function doesn't
> let callers specify access permissions, so I would consider that if a
> mapping is present on the gfn and it already points to the requested
> mfn no error should be returned to the caller. At the end the 'default
> access' for that gfn -> mfn relation is the one established by
> set_identity_p2m_entry and shouldn't be subject to the p2m default
> access.

I think further reducing access is in theory supposed to be possible.
For Dom0 all of this (including the potential of default access not
being RWX) a questionable thing though, as pointed out in earlier
discussions. After all there's no introspection (or alike) agent
supposed to be controlling Dom0.

Jan