[PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Julien Grall]

The code to initialize the grant table in libxc uses
xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
frame and to initialize it.

This solution has two major issues:
    - The check of the return of xc_domain_maximum_gpfn is buggy because
    xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
    Which is never catch with ( pfn <= 0 ).
    - The guest memory layout maybe filled up to the end, i.e
    xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
    hardware limitation.

Futhermore, on ARM, xc_domain_maximum_gpfn() is not implemented and
return -ENOSYS. This will make libxc to use always the same PFN which
may colapse with an already mapped region (see xen/include/public/arch-arm.h
for the layout).

This patch only address the problem for ARM, the x86 version use the same
behavior (ie xc_domain_maximum_gpfn() + 1), as I'm not familiar with Xen x86.

A new function xc_core_arch_get_scratch_gpfn is introduced to be able to
choose the gpfn per architecture.

For the ARM version, we use the GUEST_GNTTAB_GUEST which is the base of
the region by the guest to map the grant table. At the build time,
nothing is mapped there.

At the same time correctly check the return of xc_domain_maximum_gpfn
for x86.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

---
    I chose to take this appproach after the discussion on implementing
    XENMEM_maximum_gpfn on ARM (https://patches.linaro.org/32894/).

    This patch has only been built tested on x86 and the same behavior
    has been kept (i.e xc_domain_maximum_gpfn() + 1). I would be happy
    if someone for x86 world is looking for a possible solution.
---
 tools/libxc/xc_core.h     |  3 +++
 tools/libxc/xc_core_arm.c | 17 +++++++++++++++++
 tools/libxc/xc_core_x86.c | 17 +++++++++++++++++
 tools/libxc/xc_dom_boot.c | 18 ++++++++++++------
 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_core.h b/tools/libxc/xc_core.h
index 10cbfca..5867030 100644
--- a/tools/libxc/xc_core.h
+++ b/tools/libxc/xc_core.h
@@ -148,6 +148,9 @@ int xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width,
                                   shared_info_any_t *live_shinfo,
                                   xen_pfn_t **live_p2m, unsigned long *pfnp);
 
+int xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
+                                  xen_pfn_t *gpfn);
+
 
 #if defined (__i386__) || defined (__x86_64__)
 # include "xc_core_x86.h"
diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index 2fbcf3f..4c34191 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -96,6 +96,23 @@ xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_do
     return xc_core_arch_map_p2m_rw(xch, dinfo, info,
                                    live_shinfo, live_p2m, pfnp, 1);
 }
+
+int
+xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
+                              xen_pfn_t *gpfn)
+{
+    /*
+     * The Grant Table region space is not used until the guest is
+     * booting. Use the first page for the scrach pfn.
+     */
+    XC_BUILD_BUG_ON(GUEST_GNTTAB_SIZE < XC_PAGE_SIZE);
+
+    *gpfn = GUEST_GNTTAB_BASE >> XC_PAGE_SHIFT;
+
+    return 0;
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index f05060a..b157d85 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -205,6 +205,23 @@ xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_do
     return xc_core_arch_map_p2m_rw(xch, dinfo, info,
                                    live_shinfo, live_p2m, pfnp, 1);
 }
+
+int
+xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
+                              xen_pfn_t *gpfn)
+{
+    int rc;
+
+    rc = xc_domain_maximum_gpfn(xch, domid);
+
+    if ( rc <= 0 )
+        return rc;
+
+    *gpfn = rc;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index f0a1c64..a141eb5 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -33,6 +33,7 @@
 
 #include "xg_private.h"
 #include "xc_dom.h"
+#include "xc_core.h"
 #include <xen/hvm/params.h>
 #include <xen/grant_table.h>
 
@@ -365,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
                            domid_t xenstore_domid)
 {
     int rc;
-    xen_pfn_t max_gfn;
+    xen_pfn_t scratch_gpfn;
     struct xen_add_to_physmap xatp = {
         .domid = domid,
         .space = XENMAPSPACE_grant_table,
@@ -375,16 +376,21 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
         .domid = domid,
     };
 
-    max_gfn = xc_domain_maximum_gpfn(xch, domid);
-    if ( max_gfn <= 0 ) {
+    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
+    if ( rc < 0 )
+    {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to get max gfn "
+                     "%s: failed to get a scratch gfn "
                      "[errno=%d]\n",
                      __FUNCTION__, errno);
         return -1;
     }
-    xatp.gpfn = max_gfn + 1;
-    xrfp.gpfn = max_gfn + 1;
+    xatp.gpfn = scratch_gpfn;
+    xrfp.gpfn = scratch_gpfn;
+
+    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
+                  scratch_gpfn);
+
 
     rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
     if ( rc != 0 )
-- 
2.1.4


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

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Ian Campbell]

On Tue, 2015-01-13 at 20:10 +0000, Julien Grall wrote:

> +int
> +xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
> +                              xen_pfn_t *gpfn)
> +{
> +    /*
> +     * The Grant Table region space is not used until the guest is
> +     * booting. Use the first page for the scrach pfn.

"scratch".

> +     */
> +    XC_BUILD_BUG_ON(GUEST_GNTTAB_SIZE < XC_PAGE_SIZE);
> +
> +    *gpfn = GUEST_GNTTAB_BASE >> XC_PAGE_SHIFT;
> +
> +    return 0;
> +}
> +
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> index f05060a..b157d85 100644
> --- a/tools/libxc/xc_core_x86.c
> +++ b/tools/libxc/xc_core_x86.c
> @@ -205,6 +205,23 @@ xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_do
>      return xc_core_arch_map_p2m_rw(xch, dinfo, info,
>                                     live_shinfo, live_p2m, pfnp, 1);
>  }
> +
> +int
> +xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
> +                              xen_pfn_t *gpfn)
> +{
> +    int rc;
> +
> +    rc = xc_domain_maximum_gpfn(xch, domid);
> +
> +    if ( rc <= 0 )
> +        return rc;
> +
> +    *gpfn = rc;

Shouldn't this be rc + 1 to match the old behaviour?

> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index f0a1c64..a141eb5 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -33,6 +33,7 @@
>  
>  #include "xg_private.h"
>  #include "xc_dom.h"
> +#include "xc_core.h"
>  #include <xen/hvm/params.h>
>  #include <xen/grant_table.h>
>  
> @@ -365,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>                             domid_t xenstore_domid)
>  {
>      int rc;
> -    xen_pfn_t max_gfn;
> +    xen_pfn_t scratch_gpfn;
>      struct xen_add_to_physmap xatp = {
>          .domid = domid,
>          .space = XENMAPSPACE_grant_table,
> @@ -375,16 +376,21 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>          .domid = domid,
>      };
>  
> -    max_gfn = xc_domain_maximum_gpfn(xch, domid);
> -    if ( max_gfn <= 0 ) {
> +    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
> +    if ( rc < 0 )
> +    {
>          xc_dom_panic(xch, XC_INTERNAL_ERROR,
> -                     "%s: failed to get max gfn "
> +                     "%s: failed to get a scratch gfn "
>                       "[errno=%d]\n",
>                       __FUNCTION__, errno);
>          return -1;
>      }
> -    xatp.gpfn = max_gfn + 1;
> -    xrfp.gpfn = max_gfn + 1;
> +    xatp.gpfn = scratch_gpfn;
> +    xrfp.gpfn = scratch_gpfn;
> +
> +    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
> +                  scratch_gpfn);
> +
>  
>      rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
>      if ( rc != 0 )

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Andrew Cooper]

On 13/01/15 20:10, Julien Grall wrote:
> The code to initialize the grant table in libxc uses
> xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
> frame and to initialize it.
>
> This solution has two major issues:
>     - The check of the return of xc_domain_maximum_gpfn is buggy because
>     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
>     Which is never catch with ( pfn <= 0 ).

Wow - xc_domain_maximum_gpfn() will currently truncate long to int on
64bit systems.  That is unhelpful of it.

>     - The guest memory layout maybe filled up to the end, i.e
>     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
>     hardware limitation.

I realise I am throwing a spanner in the works here, but if you are
looking to fix it, lets fix this properly rather than hacking around the
problem further.

There is currently no way for the toolstack to map something on behalf
of a guest which is not in the guest physmap.  As a workaround, the code
here shoots a guest ram page, adds a non-ram page to the physmap, maps,
edits, unmaps and replaces the ram.

This is inefficient, liable to shatter superpages, and likely to end up
with with the returned ram allocated from the wrong numa node.

Furthermore, we have had security vulnerabilities in the past because
toolstack/device model components use guest pages (because of no
alternate mechanism) for emulation state/ring buffers without preventing
the guest itself from having access if it can find them.

Both of these issues are caused by the underlying inability for the
toolstack to map anything other than gfn space.

In the general case, an "alloc/map emulation page for" interface would
fix the security issue side of things (and make some existing code far
more simple).

However, in this case a mapping hypercall for pages which a guest could
add to its own physmap (but are not necessarily present at the moment)
would be the correct solution.

~Andrew

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Julien Grall]

On 14/01/15 12:58, Andrew Cooper wrote:
> On 13/01/15 20:10, Julien Grall wrote:
>> The code to initialize the grant table in libxc uses
>> xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
>> frame and to initialize it.
>>
>> This solution has two major issues:
>>     - The check of the return of xc_domain_maximum_gpfn is buggy because
>>     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
>>     Which is never catch with ( pfn <= 0 ).
> 
> Wow - xc_domain_maximum_gpfn() will currently truncate long to int on
> 64bit systems.  That is unhelpful of it.
> 
>>     - The guest memory layout maybe filled up to the end, i.e
>>     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
>>     hardware limitation.
> 
> I realise I am throwing a spanner in the works here, but if you are
> looking to fix it, lets fix this properly rather than hacking around the
> problem further.
> 
> There is currently no way for the toolstack to map something on behalf
> of a guest which is not in the guest physmap.  As a workaround, the code
> here shoots a guest ram page, adds a non-ram page to the physmap, maps,
> edits, unmaps and replaces the ram.

Hmmm... why do you talk about shooting a guest RAM page? Neither the
current code, nor the suggested solution for ARM does a such things.

ARM is defining a grant table range which is out of the RAM and not
mapped at that time. So we can reuse it with any possible issue.

Regards,

-- 
Julien Grall

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Andrew Cooper]

On 14/01/15 13:20, Julien Grall wrote:
> On 14/01/15 12:58, Andrew Cooper wrote:
>> On 13/01/15 20:10, Julien Grall wrote:
>>> The code to initialize the grant table in libxc uses
>>> xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
>>> frame and to initialize it.
>>>
>>> This solution has two major issues:
>>>     - The check of the return of xc_domain_maximum_gpfn is buggy because
>>>     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
>>>     Which is never catch with ( pfn <= 0 ).
>> Wow - xc_domain_maximum_gpfn() will currently truncate long to int on
>> 64bit systems.  That is unhelpful of it.
>>
>>>     - The guest memory layout maybe filled up to the end, i.e
>>>     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
>>>     hardware limitation.
>> I realise I am throwing a spanner in the works here, but if you are
>> looking to fix it, lets fix this properly rather than hacking around the
>> problem further.
>>
>> There is currently no way for the toolstack to map something on behalf
>> of a guest which is not in the guest physmap.  As a workaround, the code
>> here shoots a guest ram page, adds a non-ram page to the physmap, maps,
>> edits, unmaps and replaces the ram.
> Hmmm... why do you talk about shooting a guest RAM page? Neither the
> current code, nor the suggested solution for ARM does a such things.

That is what x86 does.  I assumed (clearly incorrectly) that ARM was
similar.

>
> ARM is defining a grant table range which is out of the RAM and not
> mapped at that time. So we can reuse it with any possible issue.

Do you mean to say that there is a grant table range baked into the ABI
occupying guest physical address space?

~Andrew

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Julien Grall]

On 14/01/15 13:23, Andrew Cooper wrote:
> On 14/01/15 13:20, Julien Grall wrote:
>> On 14/01/15 12:58, Andrew Cooper wrote:
>>> On 13/01/15 20:10, Julien Grall wrote:
>>>> The code to initialize the grant table in libxc uses
>>>> xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
>>>> frame and to initialize it.
>>>>
>>>> This solution has two major issues:
>>>>     - The check of the return of xc_domain_maximum_gpfn is buggy because
>>>>     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
>>>>     Which is never catch with ( pfn <= 0 ).
>>> Wow - xc_domain_maximum_gpfn() will currently truncate long to int on
>>> 64bit systems.  That is unhelpful of it.
>>>
>>>>     - The guest memory layout maybe filled up to the end, i.e
>>>>     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
>>>>     hardware limitation.
>>> I realise I am throwing a spanner in the works here, but if you are
>>> looking to fix it, lets fix this properly rather than hacking around the
>>> problem further.
>>>
>>> There is currently no way for the toolstack to map something on behalf
>>> of a guest which is not in the guest physmap.  As a workaround, the code
>>> here shoots a guest ram page, adds a non-ram page to the physmap, maps,
>>> edits, unmaps and replaces the ram.
>> Hmmm... why do you talk about shooting a guest RAM page? Neither the
>> current code, nor the suggested solution for ARM does a such things.
> 
> That is what x86 does.  I assumed (clearly incorrectly) that ARM was
> similar.

Are you sure? With xc_domain_maximum_gpfn() + 1 we should not shoot a
RAM page. At least most of the time.

>>
>> ARM is defining a grant table range which is out of the RAM and not
>> mapped at that time. So we can reuse it with any possible issue.
> 
> Do you mean to say that there is a grant table range baked into the ABI
> occupying guest physical address space?

Yes.

Regards,

-- 
Julien Grall

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Ian Campbell]

On Wed, 2015-01-14 at 13:23 +0000, Andrew Cooper wrote:
> > ARM is defining a grant table range which is out of the RAM and not
> > mapped at that time. So we can reuse it with any possible issue.
> 
> Do you mean to say that there is a grant table range baked into the ABI
> occupying guest physical address space?

A range of otherwise unused IPA space is suggested to the guest via the
device tree node, as a convenience for the guest so it doesn't need to
figure one out for itself (it's morally equivalent to the BAR provided
on the x86 platform PCI device).

So the actual address is not part of the guest ABI, but the toolstack
does know what it is going to be since it is privy to some internal
details of the guest layout.

Ian.

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Julien Grall]

Hi Ian,

On 14/01/15 11:03, Ian Campbell wrote:
>> +int
>> +xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
>> +                              xen_pfn_t *gpfn)
>> +{
>> +    int rc;
>> +
>> +    rc = xc_domain_maximum_gpfn(xch, domid);
>> +
>> +    if ( rc <= 0 )
>> +        return rc;
>> +
>> +    *gpfn = rc;
> 
> Shouldn't this be rc + 1 to match the old behaviour?

Oh right. I will fix it in the next version.

Regards,

-- 
Julien Grall

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Andrew Cooper]

On 14/01/15 13:31, Ian Campbell wrote:
> On Wed, 2015-01-14 at 13:23 +0000, Andrew Cooper wrote:
>>> ARM is defining a grant table range which is out of the RAM and not
>>> mapped at that time. So we can reuse it with any possible issue.
>> Do you mean to say that there is a grant table range baked into the ABI
>> occupying guest physical address space?
> A range of otherwise unused IPA space is suggested to the guest via the
> device tree node, as a convenience for the guest so it doesn't need to
> figure one out for itself (it's morally equivalent to the BAR provided
> on the x86 platform PCI device).
>
> So the actual address is not part of the guest ABI, but the toolstack
> does know what it is going to be since it is privy to some internal
> details of the guest layout.

Ah ok, so you can guarentee to find an area which doesn't have RAM in it.

However, being common code, the actions are still to add to the physmap,
map, edit, unmap and remove from the physmap, which is inefficient.

I still feel that the concept of a scratch gfn is flawed, and that a
mapping hypercall which takes {domid, space, idx} as parameters is the
correct solution.  (Although thinking about, this probably requires a
new privcmd ioctl? urgh)

As this change is a bugfix for ARM and makes the common situation no
worse, it is possibly acceptable on those grounds.

~Andrew

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Tim Deegan]

At 12:58 +0000 on 14 Jan (1421236725), Andrew Cooper wrote:
> On 13/01/15 20:10, Julien Grall wrote:
> > The code to initialize the grant table in libxc uses
> > xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
> > frame and to initialize it.
> >
> > This solution has two major issues:
> >     - The check of the return of xc_domain_maximum_gpfn is buggy because
> >     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
> >     Which is never catch with ( pfn <= 0 ).
> 
> Wow - xc_domain_maximum_gpfn() will currently truncate long to int on
> 64bit systems.  That is unhelpful of it.
> 
> >     - The guest memory layout maybe filled up to the end, i.e
> >     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
> >     hardware limitation.
> 
> I realise I am throwing a spanner in the works here, but if you are
> looking to fix it, lets fix this properly rather than hacking around the
> problem further.
> 
> There is currently no way for the toolstack to map something on behalf
> of a guest which is not in the guest physmap.  As a workaround, the code
> here shoots a guest ram page, adds a non-ram page to the physmap, maps,
> edits, unmaps and replaces the ram.
> 
> This is inefficient, liable to shatter superpages, and likely to end up
> with with the returned ram allocated from the wrong numa node.
> 
> Furthermore, we have had security vulnerabilities in the past because
> toolstack/device model components use guest pages (because of no
> alternate mechanism) for emulation state/ring buffers without preventing
> the guest itself from having access if it can find them.
> 
> Both of these issues are caused by the underlying inability for the
> toolstack to map anything other than gfn space.
> 
> In the general case, an "alloc/map emulation page for" interface would
> fix the security issue side of things (and make some existing code far
> more simple).

Sounds like a good idea.  Adding a new per-guest address space of
memory that is accessable to tools & xen but not to the guest, right?

Presumably the existing magic pages break down into those that the
guest can see/DMA/&c, and those that live in this other address
space.  AFAICS grant tables will be in the first category, though.

Cheers,

Tim.

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Andrew Cooper]

On 15/01/15 10:45, Tim Deegan wrote:
> At 12:58 +0000 on 14 Jan (1421236725), Andrew Cooper wrote:
>> On 13/01/15 20:10, Julien Grall wrote:
>>> The code to initialize the grant table in libxc uses
>>> xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
>>> frame and to initialize it.
>>>
>>> This solution has two major issues:
>>>     - The check of the return of xc_domain_maximum_gpfn is buggy because
>>>     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
>>>     Which is never catch with ( pfn <= 0 ).
>> Wow - xc_domain_maximum_gpfn() will currently truncate long to int on
>> 64bit systems.  That is unhelpful of it.
>>
>>>     - The guest memory layout maybe filled up to the end, i.e
>>>     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
>>>     hardware limitation.
>> I realise I am throwing a spanner in the works here, but if you are
>> looking to fix it, lets fix this properly rather than hacking around the
>> problem further.
>>
>> There is currently no way for the toolstack to map something on behalf
>> of a guest which is not in the guest physmap.  As a workaround, the code
>> here shoots a guest ram page, adds a non-ram page to the physmap, maps,
>> edits, unmaps and replaces the ram.
>>
>> This is inefficient, liable to shatter superpages, and likely to end up
>> with with the returned ram allocated from the wrong numa node.
>>
>> Furthermore, we have had security vulnerabilities in the past because
>> toolstack/device model components use guest pages (because of no
>> alternate mechanism) for emulation state/ring buffers without preventing
>> the guest itself from having access if it can find them.
>>
>> Both of these issues are caused by the underlying inability for the
>> toolstack to map anything other than gfn space.
>>
>> In the general case, an "alloc/map emulation page for" interface would
>> fix the security issue side of things (and make some existing code far
>> more simple).
> Sounds like a good idea.  Adding a new per-guest address space of
> memory that is accessable to tools & xen but not to the guest, right?

Yes, although this email contains two orthogonal suggestions.

>
> Presumably the existing magic pages break down into those that the
> guest can see/DMA/&c, and those that live in this other address
> space.  AFAICS grant tables will be in the first category, though.

Correct.

For things like grant tables, the toolstack is already capable of using
add_to_physmap to make the pages mappable, but this is inefficient and
possibly interferes with the guest physical layout.  I propose a short
circuit of this which allows the toolstack to map any legitimate physmap
spaces directly, without having to shuffle them in and out of the
physmap. i.e. a map foreign hypercall which takes {domid, space, idx} as
parameters rather than {domid, gfn}.

For the magic pages, this proposal creates a secondary address space,
which is intended never for the guest to be able to map.  This can
remove all the current "magic pages" which live in the low MMIO hole
(ioreq, bufioreq, mem_event rings, etc), and prevents the need for
emulation pages ever to be accessible to the guest.

~Andrew

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Julien Grall]

On 15/01/15 13:14, Andrew Cooper wrote:
> For things like grant tables, the toolstack is already capable of using
> add_to_physmap to make the pages mappable, but this is inefficient and
> possibly interferes with the guest physical layout.  I propose a short
> circuit of this which allows the toolstack to map any legitimate physmap
> spaces directly, without having to shuffle them in and out of the
> physmap. i.e. a map foreign hypercall which takes {domid, space, idx} as
> parameters rather than {domid, gfn}.
> 
> For the magic pages, this proposal creates a secondary address space,
> which is intended never for the guest to be able to map.  This can
> remove all the current "magic pages" which live in the low MMIO hole
> (ioreq, bufioreq, mem_event rings, etc), and prevents the need for
> emulation pages ever to be accessible to the guest.

If I'm not mistaken a such solution would require modification in the
kernel. So we would have to keep compatibility with older kernel.

Regards,

-- 
Julien Grall

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Julien Grall]

Hi,

Would this patch be suitable as a temporary solution (i.e until a better
approach is taken) in the tree?

I plan to resend a v2 with Ian's change requested.

Regards,

On 13/01/15 20:10, Julien Grall wrote:
> The code to initialize the grant table in libxc uses
> xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
> frame and to initialize it.
> 
> This solution has two major issues:
>     - The check of the return of xc_domain_maximum_gpfn is buggy because
>     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
>     Which is never catch with ( pfn <= 0 ).
>     - The guest memory layout maybe filled up to the end, i.e
>     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
>     hardware limitation.
> 
> Futhermore, on ARM, xc_domain_maximum_gpfn() is not implemented and
> return -ENOSYS. This will make libxc to use always the same PFN which
> may colapse with an already mapped region (see xen/include/public/arch-arm.h
> for the layout).
> 
> This patch only address the problem for ARM, the x86 version use the same
> behavior (ie xc_domain_maximum_gpfn() + 1), as I'm not familiar with Xen x86.
> 
> A new function xc_core_arch_get_scratch_gpfn is introduced to be able to
> choose the gpfn per architecture.
> 
> For the ARM version, we use the GUEST_GNTTAB_GUEST which is the base of
> the region by the guest to map the grant table. At the build time,
> nothing is mapped there.
> 
> At the same time correctly check the return of xc_domain_maximum_gpfn
> for x86.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
> ---
>     I chose to take this appproach after the discussion on implementing
>     XENMEM_maximum_gpfn on ARM (https://patches.linaro.org/32894/).
> 
>     This patch has only been built tested on x86 and the same behavior
>     has been kept (i.e xc_domain_maximum_gpfn() + 1). I would be happy
>     if someone for x86 world is looking for a possible solution.
> ---
>  tools/libxc/xc_core.h     |  3 +++
>  tools/libxc/xc_core_arm.c | 17 +++++++++++++++++
>  tools/libxc/xc_core_x86.c | 17 +++++++++++++++++
>  tools/libxc/xc_dom_boot.c | 18 ++++++++++++------
>  4 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxc/xc_core.h b/tools/libxc/xc_core.h
> index 10cbfca..5867030 100644
> --- a/tools/libxc/xc_core.h
> +++ b/tools/libxc/xc_core.h
> @@ -148,6 +148,9 @@ int xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width,
>                                    shared_info_any_t *live_shinfo,
>                                    xen_pfn_t **live_p2m, unsigned long *pfnp);
>  
> +int xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
> +                                  xen_pfn_t *gpfn);
> +
>  
>  #if defined (__i386__) || defined (__x86_64__)
>  # include "xc_core_x86.h"
> diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
> index 2fbcf3f..4c34191 100644
> --- a/tools/libxc/xc_core_arm.c
> +++ b/tools/libxc/xc_core_arm.c
> @@ -96,6 +96,23 @@ xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_do
>      return xc_core_arch_map_p2m_rw(xch, dinfo, info,
>                                     live_shinfo, live_p2m, pfnp, 1);
>  }
> +
> +int
> +xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
> +                              xen_pfn_t *gpfn)
> +{
> +    /*
> +     * The Grant Table region space is not used until the guest is
> +     * booting. Use the first page for the scrach pfn.
> +     */
> +    XC_BUILD_BUG_ON(GUEST_GNTTAB_SIZE < XC_PAGE_SIZE);
> +
> +    *gpfn = GUEST_GNTTAB_BASE >> XC_PAGE_SHIFT;
> +
> +    return 0;
> +}
> +
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> index f05060a..b157d85 100644
> --- a/tools/libxc/xc_core_x86.c
> +++ b/tools/libxc/xc_core_x86.c
> @@ -205,6 +205,23 @@ xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_do
>      return xc_core_arch_map_p2m_rw(xch, dinfo, info,
>                                     live_shinfo, live_p2m, pfnp, 1);
>  }
> +
> +int
> +xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
> +                              xen_pfn_t *gpfn)
> +{
> +    int rc;
> +
> +    rc = xc_domain_maximum_gpfn(xch, domid);
> +
> +    if ( rc <= 0 )
> +        return rc;
> +
> +    *gpfn = rc;
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index f0a1c64..a141eb5 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -33,6 +33,7 @@
>  
>  #include "xg_private.h"
>  #include "xc_dom.h"
> +#include "xc_core.h"
>  #include <xen/hvm/params.h>
>  #include <xen/grant_table.h>
>  
> @@ -365,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>                             domid_t xenstore_domid)
>  {
>      int rc;
> -    xen_pfn_t max_gfn;
> +    xen_pfn_t scratch_gpfn;
>      struct xen_add_to_physmap xatp = {
>          .domid = domid,
>          .space = XENMAPSPACE_grant_table,
> @@ -375,16 +376,21 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>          .domid = domid,
>      };
>  
> -    max_gfn = xc_domain_maximum_gpfn(xch, domid);
> -    if ( max_gfn <= 0 ) {
> +    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
> +    if ( rc < 0 )
> +    {
>          xc_dom_panic(xch, XC_INTERNAL_ERROR,
> -                     "%s: failed to get max gfn "
> +                     "%s: failed to get a scratch gfn "
>                       "[errno=%d]\n",
>                       __FUNCTION__, errno);
>          return -1;
>      }
> -    xatp.gpfn = max_gfn + 1;
> -    xrfp.gpfn = max_gfn + 1;
> +    xatp.gpfn = scratch_gpfn;
> +    xrfp.gpfn = scratch_gpfn;
> +
> +    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
> +                  scratch_gpfn);
> +
>  
>      rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
>      if ( rc != 0 )
> 


-- 
Julien Grall

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

Re: [PATCH] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Andrew Cooper]

On 21/01/15 12:03, Julien Grall wrote:
> Hi,
>
> Would this patch be suitable as a temporary solution (i.e until a better
> approach is taken) in the tree?
>
> I plan to resend a v2 with Ian's change requested.
>
> Regards,

Yeah - it doesn't make the problem any worse, and fixes an arm issue.

~Andrew