[PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Julien Grall]

Direct mapped domain has already the memory allocated 1:1, so we are
directly using the gfn as mfn to map the RAM in the guest.

While we are validating that the page associated to the first mfn belongs to
the domain, the subsequent MFN are not validated when the extent_order
is > 0.

This may result to map memory region (MMIO, RAM) which doesn't belong to the
domain.

Although, only DOM0 on ARM is using a direct memory mapped. So it
doesn't affect any guest (at least on the upstream version) or even x86.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>

    This patch is a candidate for Xen 4.6 and backport to Xen 4.5 (and
    maybe 4.4).

    The hypercall is used for the ballooning code and only DOM0 on ARM
    is a direct mapped domain.

    The current code to validate the GFN passed by the populate physmap
    hypercall has been moved in a for loop in order to validate each
    GFN when the extent order is > 0.

    Without it, direct mapping domain may be able to map memory region
    which doesn't belong to it.

    I think the switch to the for loop is pretty safe and contained. The
    worst thing that could happen is denying mapping more often than we
    use to do. Although, IIRC Linux is mostly mapping page one by one
    (i.e extent order = 0).
---
 xen/common/memory.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 61bb94c..7cc8af4 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -126,22 +126,28 @@ static void populate_physmap(struct memop_args *a)
             if ( is_domain_direct_mapped(d) )
             {
                 mfn = gpfn;
-                if ( !mfn_valid(mfn) )
+
+                for ( j = 0; j < (1 << a->extent_order); j++, mfn++ )
                 {
-                    gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_xen_pfn"\n",
+                    if ( !mfn_valid(mfn) )
+                    {
+                        gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_xen_pfn"\n",
                              mfn);
-                    goto out;
+                        goto out;
+                    }
+
+                    page = mfn_to_page(mfn);
+                    if ( !get_page(page, d) )
+                    {
+                        gdprintk(XENLOG_INFO,
+                                 "mfn %#"PRI_xen_pfn" doesn't belong to the"
+                                 " domain\n", mfn);
+                        goto out;
+                    }
+                    put_page(page);
                 }
 
-                page = mfn_to_page(mfn);
-                if ( !get_page(page, d) )
-                {
-                    gdprintk(XENLOG_INFO,
-                             "mfn %#"PRI_xen_pfn" doesn't belong to the"
-                             " domain\n", mfn);
-                    goto out;
-                }
-                put_page(page);
+                page = mfn_to_page(gpfn);
             }
             else
                 page = alloc_domheap_pages(d, a->extent_order, a->memflags);
-- 
2.1.4

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Jan Beulich]

>>> On 11.08.15 at 19:41, <julien.grall@citrix.com> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -126,22 +126,28 @@ static void populate_physmap(struct memop_args *a)
>              if ( is_domain_direct_mapped(d) )
>              {
>                  mfn = gpfn;
> -                if ( !mfn_valid(mfn) )
> +
> +                for ( j = 0; j < (1 << a->extent_order); j++, mfn++ )

While benign I think we shouldn't repeat mistakes like this made
elsewhere in the code: At the very least this should be 1U, but
with j (needlessly, just like i) being unsigned long it would be
more consistent for this to be 1UL unless we clean up the variable
types (which I think I'll do).

Since the change is so minor, I'd be fine with making it while
committing, unless you object.

Jan

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Wei Liu]

On Tue, Aug 11, 2015 at 06:41:28PM +0100, Julien Grall wrote:
> Direct mapped domain has already the memory allocated 1:1, so we are
> directly using the gfn as mfn to map the RAM in the guest.
> 
> While we are validating that the page associated to the first mfn belongs to
> the domain, the subsequent MFN are not validated when the extent_order
> is > 0.
> 
> This may result to map memory region (MMIO, RAM) which doesn't belong to the
> domain.
> 
> Although, only DOM0 on ARM is using a direct memory mapped. So it
> doesn't affect any guest (at least on the upstream version) or even x86.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> 
>     This patch is a candidate for Xen 4.6 and backport to Xen 4.5 (and
>     maybe 4.4).
> 

Fine by me in principle for this patch to go in 4.6.

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Julien Grall]

Hi Jan,

On 12/08/2015 10:16, Jan Beulich wrote:
>>>> On 11.08.15 at 19:41, <julien.grall@citrix.com> wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -126,22 +126,28 @@ static void populate_physmap(struct memop_args *a)
>>               if ( is_domain_direct_mapped(d) )
>>               {
>>                   mfn = gpfn;
>> -                if ( !mfn_valid(mfn) )
>> +
>> +                for ( j = 0; j < (1 << a->extent_order); j++, mfn++ )
>
> While benign I think we shouldn't repeat mistakes like this made
> elsewhere in the code: At the very least this should be 1U, but
> with j (needlessly, just like i) being unsigned long it would be
> more consistent for this to be 1UL unless we clean up the variable
> types (which I think I'll do).

I admit that I copied the loop at then end of the function without 
thinking if it there was some issue or not.

>
> Since the change is so minor, I'd be fine with making it while
> committing, unless you object.

I'm fine with that.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Wei Liu]

On Wed, Aug 12, 2015 at 10:19:04AM +0100, Wei Liu wrote:
> On Tue, Aug 11, 2015 at 06:41:28PM +0100, Julien Grall wrote:
> > Direct mapped domain has already the memory allocated 1:1, so we are
> > directly using the gfn as mfn to map the RAM in the guest.
> > 
> > While we are validating that the page associated to the first mfn belongs to
> > the domain, the subsequent MFN are not validated when the extent_order
> > is > 0.
> > 
> > This may result to map memory region (MMIO, RAM) which doesn't belong to the
> > domain.
> > 
> > Although, only DOM0 on ARM is using a direct memory mapped. So it
> > doesn't affect any guest (at least on the upstream version) or even x86.
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> > ---
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Tim Deegan <tim@xen.org>
> > 
> >     This patch is a candidate for Xen 4.6 and backport to Xen 4.5 (and
> >     maybe 4.4).
> > 
> 
> Fine by me in principle for this patch to go in 4.6.

I shall do this in a formal way.

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Ian Campbell]

On Tue, 2015-08-11 at 18:41 +0100, Julien Grall wrote:
> Direct mapped domain has already the memory allocated 1:1, so we are
> directly using the gfn as mfn to map the RAM in the guest.
> 
> While we are validating that the page associated to the first mfn belongs 
> to
> the domain, the subsequent MFN are not validated when the extent_order
> is > 0.
> 
> This may result to map memory region (MMIO, RAM) which doesn't belong to 
> the
> domain.
> 
> Although, only DOM0 on ARM is using a direct memory mapped. So it
> doesn't affect any guest (at least on the upstream version) or even x86.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> 
>     This patch is a candidate for Xen 4.6 and backport to Xen 4.5 (and
>     maybe 4.4).
> 
>     The hypercall is used for the ballooning code and only DOM0 on ARM
>     is a direct mapped domain.
> 
>     The current code to validate the GFN passed by the populate physmap
>     hypercall has been moved in a for loop in order to validate each
>     GFN when the extent order is > 0.
> 
>     Without it, direct mapping domain may be able to map memory region
>     which doesn't belong to it.
> 
>     I think the switch to the for loop is pretty safe and contained. The
>     worst thing that could happen is denying mapping more often than we
>     use to do. Although, IIRC Linux is mostly mapping page one by one
>     (i.e extent order = 0).
> ---
>  xen/common/memory.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 61bb94c..7cc8af4 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -126,22 +126,28 @@ static void populate_physmap(struct memop_args *a)
>              if ( is_domain_direct_mapped(d) )
>              {
>                  mfn = gpfn;
> -                if ( !mfn_valid(mfn) )
> +
> +                for ( j = 0; j < (1 << a->extent_order); j++, mfn++ )
>                  {
> -                    gdprintk(XENLOG_INFO, "Invalid mfn 
> %#"PRI_xen_pfn"\n",
> +                    if ( !mfn_valid(mfn) )
> +                    {
> +                        gdprintk(XENLOG_INFO, "Invalid mfn 
> %#"PRI_xen_pfn"\n",
>                               mfn);
> -                    goto out;
> +                        goto out;
> +                    }
> +
> +                    page = mfn_to_page(mfn);
> +                    if ( !get_page(page, d) )
> +                    {
> +                        gdprintk(XENLOG_INFO,
> +                                 "mfn %#"PRI_xen_pfn" doesn't belong to 
> the"
> +                                 " domain\n", mfn);
> +                        goto out;

This will leak the references on any earlier pages, which will happen all
the time if you are cross a boundary from RAM into e.g. MMIO or whatever.

A variant of get_page which took an order argument and returned either with
0 or 1<<order refs taken would perhaps make this easier to deal with. I
suppose we don't already have one of those, but you could add it.

Oh, am I wrong and we do get_page then immediately put_page for every page?
I suppose that is one way to check the current owner, but I think it needs
a comment at least (it did before TBH, just iterating over many pages makes
it seem even odder).

Is that really the best way to ask if a page is owned by a given domain tho
ugh? Does page_get_owner not suffice?

> +                    }
> +                    put_page(page);
>                  }
>  
> -                page = mfn_to_page(mfn);
> -                if ( !get_page(page, d) )
> -                {
> -                    gdprintk(XENLOG_INFO,
> -                             "mfn %#"PRI_xen_pfn" doesn't belong to the"
> -                             " domain\n", mfn);
> -                    goto out;
> -                }
> -                put_page(page);
> +                page = mfn_to_page(gpfn);
>              }
>              else
>                  page = alloc_domheap_pages(d, a->extent_order, a
> ->memflags);

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Jan Beulich]

>>> On 13.08.15 at 11:33, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-08-11 at 18:41 +0100, Julien Grall wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -126,22 +126,28 @@ static void populate_physmap(struct memop_args *a)
>>              if ( is_domain_direct_mapped(d) )
>>              {
>>                  mfn = gpfn;
>> -                if ( !mfn_valid(mfn) )
>> +
>> +                for ( j = 0; j < (1 << a->extent_order); j++, mfn++ )
>>                  {
>> -                    gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_xen_pfn"\n",
>> +                    if ( !mfn_valid(mfn) )
>> +                    {
>> +                        gdprintk(XENLOG_INFO, "Invalid mfn %#"PRI_xen_pfn"\n",
>>                               mfn);
>> -                    goto out;
>> +                        goto out;
>> +                    }
>> +
>> +                    page = mfn_to_page(mfn);
>> +                    if ( !get_page(page, d) )
>> +                    {
>> +                        gdprintk(XENLOG_INFO,
>> +                                 "mfn %#"PRI_xen_pfn" doesn't belong to the"
>> +                                 " domain\n", mfn);
>> +                        goto out;
> 
> This will leak the references on any earlier pages, which will happen all
> the time if you are cross a boundary from RAM into e.g. MMIO or whatever.

I don't see why it would - the put_page() (as you say a few lines
down in your reply) follows right afterwards.

> A variant of get_page which took an order argument and returned either with
> 0 or 1<<order refs taken would perhaps make this easier to deal with. I
> suppose we don't already have one of those, but you could add it.

That would be useful if we wanted to retain the ref, but that's not the
case here.

> Oh, am I wrong and we do get_page then immediately put_page for every page?
> I suppose that is one way to check the current owner, but I think it needs
> a comment at least (it did before TBH, just iterating over many pages makes
> it seem even odder).
> 
> Is that really the best way to ask if a page is owned by a given domain tho
> ugh? Does page_get_owner not suffice?

That could produce wrong results when the page previously had
no reference (and namely, due to the unions used in
struct page_info, was free or in use as a shadow page).

Jan

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Ian Campbell]

On Thu, 2015-08-13 at 03:54 -0600, Jan Beulich wrote:
> > 
> > > > On 13.08.15 at 11:33, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-08-11 at 18:41 +0100, Julien Grall wrote:
> > > --- a/xen/common/memory.c
> > > +++ b/xen/common/memory.c
> > > @@ -126,22 +126,28 @@ static void populate_physmap(struct memop_args 
> > > *a)
> > >              if ( is_domain_direct_mapped(d) )
> > >              {
> > >                  mfn = gpfn;
> > > -                if ( !mfn_valid(mfn) )
> > > +
> > > +                for ( j = 0; j < (1 << a->extent_order); j++, mfn++ 
> > > )
> > >                  {
> > > -                    gdprintk(XENLOG_INFO, "Invalid mfn 
> > > %#"PRI_xen_pfn"\n",
> > > +                    if ( !mfn_valid(mfn) )
> > > +                    {
> > > +                        gdprintk(XENLOG_INFO, "Invalid mfn 
> > > %#"PRI_xen_pfn"\n",
> > >                               mfn);
> > > -                    goto out;
> > > +                        goto out;
> > > +                    }
> > > +
> > > +                    page = mfn_to_page(mfn);
> > > +                    if ( !get_page(page, d) )
> > > +                    {
> > > +                        gdprintk(XENLOG_INFO,
> > > +                                 "mfn %#"PRI_xen_pfn" doesn't belong 
> > > to the"
> > > +                                 " domain\n", mfn);
> > > +                        goto out;
> > 
> > This will leak the references on any earlier pages, which will happen 
> > all
> > the time if you are cross a boundary from RAM into e.g. MMIO or 
> > whatever.
> 
> I don't see why it would - the put_page() (as you say a few lines
> down in your reply) follows right afterwards.

Right, sorry I should have delete all this once I realised what was going
on (I wasn't 100% sure I guess).

> > Oh, am I wrong and we do get_page then immediately put_page for every page?
> > I suppose that is one way to check the current owner, but I think it needs
> > a comment at least (it did before TBH, just iterating over many pages makes
> > it seem even odder).
> > 
> > Is that really the best way to ask if a page is owned by a given domain tho
> > ugh? Does page_get_owner not suffice?
> 
> That could produce wrong results when the page previously had
> no reference (and namely, due to the unions used in
> struct page_info, was free or in use as a shadow page).

I see.

Maybe some new helper to encapsulate the get+put to validate the presence
of a current owner would be nice in the future then, but not a blocker for
this patch I guess.

There isn't a race here is there? What if the reference were dropped after
this check but before the guest_physmap_add_page (which takes new
references)? We are implicitly relying on a guarantee made elsewhere that a
direct mapped guest never drops the final ref until it is destroyed (which
can't be happening in this window I think), but it would be obviously safer
against such bugs if we just held the ref until after the p2m was updated.

Another thing for a future/4.7 cleanup I guess.

Ian.

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Jan Beulich]

>>> On 13.08.15 at 12:14, <ian.campbell@citrix.com> wrote:
> There isn't a race here is there? What if the reference were dropped after
> this check but before the guest_physmap_add_page (which takes new
> references)? We are implicitly relying on a guarantee made elsewhere that a
> direct mapped guest never drops the final ref until it is destroyed (which
> can't be happening in this window I think), but it would be obviously safer
> against such bugs if we just held the ref until after the p2m was updated.

Yeah, we depend on this, and what you suggest would indeed be
an improvment.

> Another thing for a future/4.7 cleanup I guess.

Right.

But as bottom line I take it that your objections to the patch going in
soon have been eliminated?

Jan

Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Ian Campbell]

On Thu, 2015-08-13 at 04:29 -0600, Jan Beulich wrote:
> > 
> > > > On 13.08.15 at 12:14, <ian.campbell@citrix.com> wrote:
> > There isn't a race here is there? What if the reference were dropped 
> > after
> > this check but before the guest_physmap_add_page (which takes new
> > references)? We are implicitly relying on a guarantee made elsewhere 
> > that a
> > direct mapped guest never drops the final ref until it is destroyed 
> > (which
> > can't be happening in this window I think), but it would be obviously 
> > safer
> > against such bugs if we just held the ref until after the p2m was 
> > updated.
> 
> Yeah, we depend on this, and what you suggest would indeed be
> an improvment.
> 
> > Another thing for a future/4.7 cleanup I guess.
> 
> Right.
> 
> But as bottom line I take it that your objections to the patch going in
> soon have been eliminated?

Correct.

Acked-by: Ian Campbell <ian.campbell@citrix.com>