Re: [PATCH v2 2/6] x86/P2M: relax permissions of PVH Dom0's MMIO entries

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
> 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);
>
>