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@suse.com>
---
v2: Split off from original patch.

--- 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
+         * be independent of the sequence of operations.
+         */
+        if ( is_hardware_domain(d) && gfn_p2mt == p2m_mmio_direct &&
+             access <= p2m_access_rwx && a <= p2m_access_rwx )
+            access |= a;
+
         if ( access == a )
         {
             gfn_unlock(p2m, gfn, order);

On 02/09/2021 09:33, 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.

?

That's literally the opposite of what needs to happen to fix this bug. 
Introspection is the only interface which should be restricting X
permissions.

>  Plus the use of p2m_access_* here is abusive in the
> first place.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Split off from original patch.
>
> --- 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
> +         * be independent of the sequence of operations.

"result to be"

~Andrew

> +         */
> +        if ( is_hardware_domain(d) && gfn_p2mt == p2m_mmio_direct &&
> +             access <= p2m_access_rwx && a <= p2m_access_rwx )
> +            access |= a;
> +
>          if ( access == a )
>          {
>              gfn_unlock(p2m, gfn, order);
>
>

On 06.09.2021 17:48, Andrew Cooper wrote:
> On 02/09/2021 09:33, 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.
> 
> ?
> 
> That's literally the opposite of what needs to happen to fix this bug. 
> Introspection is the only interface which should be restricting X
> permissions.

What agent would be handling access violations in Dom0? The description
(now) focuses on entirely Dom0; I agree that DomU wants things the way
you describe (provided all p2m_access_t abuses are gone).

Jan

On 06/09/2021 16:55, Jan Beulich wrote:
> On 06.09.2021 17:48, Andrew Cooper wrote:
>> On 02/09/2021 09:33, 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.
>> ?
>>
>> That's literally the opposite of what needs to happen to fix this bug. 
>> Introspection is the only interface which should be restricting X
>> permissions.
> What agent would be handling access violations in Dom0?

None.  dom0 really shouldn't have any NX mappings in the first place.

~Andrew