Re: [Xen-devel] [PATCH v2 1/4] x86/PV: tighten page table ownership check in emul-priv-op.c:read_cr()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 04, 2019 at 06:41:29AM -0600, Jan Beulich wrote:
> Rather than checking that a page table is _not_ "owned" by the fake COW
> domain, check that it's owned by the domain actually wanting to install
> it.
> 
> Switch away from BUG_ON() at the same time.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> v2: Split out from larger patch to make further adjustments.
> ---
> Thinking about it I wonder why we have such a check here and no-where
> else. An alternative would seem to be to simply drop the BUG_ON().
> 
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -706,7 +706,7 @@ static int read_cr(unsigned int reg, uns
>  
>      case 3: /* Read CR3 */
>      {
> -        const struct domain *currd = curr->domain;
> +        struct domain *currd = curr->domain;
>          mfn_t mfn;
>  
>          if ( !is_pv_32bit_domain(currd) )
> @@ -723,8 +723,14 @@ static int read_cr(unsigned int reg, uns
>              unmap_domain_page(pl4e);
>              *val = compat_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn)));
>          }
> -        /* PTs should not be shared */
> -        BUG_ON(page_get_owner(mfn_to_page(mfn)) == dom_cow);
> +
> +        /* PTs should be owned by their domains */
> +        if ( page_get_owner(mfn_to_page(mfn)) != currd )
> +        {
> +            ASSERT_UNREACHABLE();
> +            domain_crash(currd);

I wonder whether you could keep currd as const and just use
curr->domain here.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel