[PATCH] x86/mm: relax the check in get_pg_owner

[PATCH] x86/mm: relax the check in get_pg_owner

[Wei Liu]

PVH guest is actually an translated guest. It should be able to
manipulate page table for other domains when acting as Dom0.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/mm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index a6b2649cda..3635764df8 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3041,12 +3041,6 @@ static struct domain *get_pg_owner(domid_t domid)
         goto out;
     }
 
-    if ( unlikely(paging_mode_translate(curr)) )
-    {
-        MEM_LOG("Cannot mix foreign mappings with translated domains");
-        goto out;
-    }
-
     switch ( domid )
     {
     case DOMID_IO:
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/mm: relax the check in get_pg_owner

[Jan Beulich]

>>> On 23.03.17 at 19:08, <wei.liu2@citrix.com> wrote:
> PVH guest is actually an translated guest. It should be able to
> manipulate page table for other domains when acting as Dom0.

The same was true for PVHv1, so I'm afraid there's a little more to
this.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3041,12 +3041,6 @@ static struct domain *get_pg_owner(domid_t domid)
>          goto out;
>      }
>  
> -    if ( unlikely(paging_mode_translate(curr)) )
> -    {
> -        MEM_LOG("Cannot mix foreign mappings with translated domains");
> -        goto out;
> -    }

Prior to Roger's recent removal of PVHv1 code this was

    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )

Instead of removing the left side, I think it should have been
converted to !is_hvm_domain() (or is_pv_domain()).

Protection against this being used on other than PV domains
as target luckily looks to be there:
- mmuext and mmu_update already have respective (albeit
  somewhat inconsistent) checks,
- do_update_va_mapping_otherdomain() is not wired up for
  HVM.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/mm: relax the check in get_pg_owner

[Wei Liu]

On Fri, Mar 24, 2017 at 02:12:47AM -0600, Jan Beulich wrote:
> >>> On 23.03.17 at 19:08, <wei.liu2@citrix.com> wrote:
> > PVH guest is actually an translated guest. It should be able to
> > manipulate page table for other domains when acting as Dom0.
> 
> The same was true for PVHv1, so I'm afraid there's a little more to
> this.
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -3041,12 +3041,6 @@ static struct domain *get_pg_owner(domid_t domid)
> >          goto out;
> >      }
> >  
> > -    if ( unlikely(paging_mode_translate(curr)) )
> > -    {
> > -        MEM_LOG("Cannot mix foreign mappings with translated domains");
> > -        goto out;
> > -    }
> 
> Prior to Roger's recent removal of PVHv1 code this was
> 
>     if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
> 
> Instead of removing the left side, I think it should have been
> converted to !is_hvm_domain() (or is_pv_domain()).
> 

DYM:

    if ( !is_hvm_domain(curr) && unlikely(paging_mode_translate(curr)) )

?

Here is something I'm not entirely sure of: is autotranslated PV guest
still a thing? I thought Andrew removed it already?

> Protection against this being used on other than PV domains
> as target luckily looks to be there:
> - mmuext and mmu_update already have respective (albeit
>   somewhat inconsistent) checks,
> - do_update_va_mapping_otherdomain() is not wired up for
>   HVM.

Yes. That's right.

Wei.

> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/mm: relax the check in get_pg_owner

[Jan Beulich]

>>> On 24.03.17 at 12:08, <wei.liu2@citrix.com> wrote:
> On Fri, Mar 24, 2017 at 02:12:47AM -0600, Jan Beulich wrote:
>> >>> On 23.03.17 at 19:08, <wei.liu2@citrix.com> wrote:
>> > PVH guest is actually an translated guest. It should be able to
>> > manipulate page table for other domains when acting as Dom0.
>> 
>> The same was true for PVHv1, so I'm afraid there's a little more to
>> this.
>> 
>> > --- a/xen/arch/x86/mm.c
>> > +++ b/xen/arch/x86/mm.c
>> > @@ -3041,12 +3041,6 @@ static struct domain *get_pg_owner(domid_t domid)
>> >          goto out;
>> >      }
>> >  
>> > -    if ( unlikely(paging_mode_translate(curr)) )
>> > -    {
>> > -        MEM_LOG("Cannot mix foreign mappings with translated domains");
>> > -        goto out;
>> > -    }
>> 
>> Prior to Roger's recent removal of PVHv1 code this was
>> 
>>     if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
>> 
>> Instead of removing the left side, I think it should have been
>> converted to !is_hvm_domain() (or is_pv_domain()).
>> 
> 
> DYM:
> 
>     if ( !is_hvm_domain(curr) && unlikely(paging_mode_translate(curr)) )
> 
> ?

Yes.

> Here is something I'm not entirely sure of: is autotranslated PV guest
> still a thing? I thought Andrew removed it already?

From the tools, iirc, but not from the hypervisor, at least not
entirely (as you see from the change you want to make).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/mm: relax the check in get_pg_owner

[Wei Liu]

On Fri, Mar 24, 2017 at 05:17:51AM -0600, Jan Beulich wrote:
> >>> On 24.03.17 at 12:08, <wei.liu2@citrix.com> wrote:
> > On Fri, Mar 24, 2017 at 02:12:47AM -0600, Jan Beulich wrote:
> >> >>> On 23.03.17 at 19:08, <wei.liu2@citrix.com> wrote:
> >> > PVH guest is actually an translated guest. It should be able to
> >> > manipulate page table for other domains when acting as Dom0.
> >> 
> >> The same was true for PVHv1, so I'm afraid there's a little more to
> >> this.
> >> 
> >> > --- a/xen/arch/x86/mm.c
> >> > +++ b/xen/arch/x86/mm.c
> >> > @@ -3041,12 +3041,6 @@ static struct domain *get_pg_owner(domid_t domid)
> >> >          goto out;
> >> >      }
> >> >  
> >> > -    if ( unlikely(paging_mode_translate(curr)) )
> >> > -    {
> >> > -        MEM_LOG("Cannot mix foreign mappings with translated domains");
> >> > -        goto out;
> >> > -    }
> >> 
> >> Prior to Roger's recent removal of PVHv1 code this was
> >> 
> >>     if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
> >> 
> >> Instead of removing the left side, I think it should have been
> >> converted to !is_hvm_domain() (or is_pv_domain()).
> >> 
> > 
> > DYM:
> > 
> >     if ( !is_hvm_domain(curr) && unlikely(paging_mode_translate(curr)) )
> > 
> > ?
> 
> Yes.
> 
> > Here is something I'm not entirely sure of: is autotranslated PV guest
> > still a thing? I thought Andrew removed it already?
> 
> From the tools, iirc, but not from the hypervisor, at least not
> entirely (as you see from the change you want to make).
> 

OK, I will update the patch per your suggestion.

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel