Re: [Xen-Devel] [PATCH] [GSOC14] refactored mempaging code from xenpaging to libxc.

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 10937 bytes --]

On 13/06/14 16:07, Andres Lagar Cavilla wrote:
> On Fri, Jun 13, 2014 at 7:51 AM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     On 13/06/14 15:16, Dushyant Behl wrote:
>     > This patch is part of the work done under the gsoc project -
>     > Lazy Restore Using Memory Paging.
>     >
>     > This patch moves the code to initialize mempaging from xenpaging
>     to libxc.
>     > The code refractored from xenpaging is the code which sets up
>     paging,
>     > initializes a shared ring and event channel to communicate with
>     xen. This
>     > communication is done between the hypervisor and the dom0 tool
>     which performs
>     > the activity of pager. The xenpaging code is changed to use the
>     newly created
>     > routines and is tested to properly build and work with this code.
>     >
>     > The refractoring is done so that any tool which will act as pager in
>     > lazy restore or use memory paging can use a same routines to
>     initialize mempaging.
>     > This refactoring will also allow any future (in-tree) tools to
>     use mempaging.
>     >
>     > The refractored code in xc_mem_paging_ring_setup is to be
>     compiled into
>     > libxenguest.
>     >
>     > Signed-off-by: Dushyant Behl <myselfdushyantbehl@gmail.com
>     <mailto:myselfdushyantbehl@gmail.com>>
>     > Reviewed-by: Andres Lagar-Cavilla <andres@lagarcavilla.org
>     <mailto:andres@lagarcavilla.org>>
>     > Acked-by: David Scott <dave.scott@citrix.com
>     <mailto:dave.scott@citrix.com>>
>     > ---
>     >  tools/libxc/Makefile                |   2 +
>     >  tools/libxc/xc_mem_paging_setup.c   | 135
>     ++++++++++++++++++++++++++++++++++++
>     >  tools/libxc/xenctrl.h               |  15 ++++
>     >  tools/ocaml/libs/xc/xenctrl_stubs.c |  11 +--
>     >  tools/xenpaging/Makefile            |   4 +-
>     >  tools/xenpaging/xenpaging.c         |  93 +++----------------------
>     >  6 files changed, 172 insertions(+), 88 deletions(-)
>     >  create mode 100644 tools/libxc/xc_mem_paging_setup.c
>     >
>     > diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
>     > index a74b19e..6cf14f0 100644
>     > --- a/tools/libxc/Makefile
>     > +++ b/tools/libxc/Makefile
>     > @@ -69,6 +69,8 @@ GUEST_SRCS-$(CONFIG_ARM)     +=
>     xc_dom_armzimageloader.c
>     >  GUEST_SRCS-y                 += xc_dom_binloader.c
>     >  GUEST_SRCS-y                 += xc_dom_compat_linux.c
>     >
>     > +GUEST_SRCS-y                 += xc_mem_paging_setup.c
>     > +
>     >  GUEST_SRCS-$(CONFIG_X86)     += xc_dom_x86.c
>     >  GUEST_SRCS-$(CONFIG_X86)     += xc_cpuid_x86.c
>     >  GUEST_SRCS-$(CONFIG_X86)     += xc_hvm_build_x86.c
>     > diff --git a/tools/libxc/xc_mem_paging_setup.c
>     b/tools/libxc/xc_mem_paging_setup.c
>     > new file mode 100644
>     > index 0000000..7b3ab38
>     > --- /dev/null
>     > +++ b/tools/libxc/xc_mem_paging_setup.c
>     > @@ -0,0 +1,135 @@
>     > +/*
>     > + * tools/libxc/xc_mem_paging_setup.c
>     > + *
>     > + * Routines to initialize memory paging. Create shared ring
>     > + * and event channels to communicate with the hypervisor.
>     > + *
>     > + * Copyright (c) 2014 Dushyant Behl
>     > + * Copyright (c) 2009 by Citrix Systems, Inc. (Patrick Colp)
>     > + *
>     > + * This library is free software; you can redistribute it and/or
>     > + * modify it under the terms of the GNU Lesser General Public
>     > + * License as published by the Free Software Foundation; either
>     > + * version 2.1 of the License, or (at your option) any later
>     version.
>     > + *
>     > + * This library is distributed in the hope that it will be useful,
>     > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>     > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
>     the GNU
>     > + * Lesser General Public License for more details.
>     > + *
>     > + * You should have received a copy of the GNU Lesser General Public
>     > + * License along with this library; if not, write to the Free
>     Software
>     > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>     MA  02110-1301 USA
>     > + */
>     > +
>     > +#include "xc_private.h"
>     > +#include <xen/event_channel.h>
>     > +#include <xen/mem_event.h>
>     > +
>     > +/*
>     > + * Mem paging ring and event channel setup routine.
>     > + * Setup a shared ring and an event channel to communicate between
>     > + * hypervisor and the tool performing mem paging operations.
>     > + * The function will return zero on successful completion and will
>     > + * return -1 on failure at any intermediate step setting up errno
>     > + * properly.
>     > + */
>     > +int xc_mem_paging_ring_setup(xc_interface *xch, domid_t domain_id,
>     > +                                void *ring_page, int *port,
>     > +                                uint32_t *evtchn_port,
>     > +                                xc_evtchn **xce_handle,
>     > +                                mem_event_back_ring_t *back_ring)
>     > +{
>     > +    int rc;
>     > +    uint64_t ring_pfn, mmap_pfn;
>     > +
>     > +    /* Map the ring page */
>     > +    xc_get_hvm_param(xch, domain_id, HVM_PARAM_PAGING_RING_PFN,
>     &ring_pfn);
>     > +    mmap_pfn = ring_pfn;
>     > +    ring_page = xc_map_foreign_batch(xch, domain_id,
>     > +                                        PROT_READ | PROT_WRITE,
>     &mmap_pfn, 1);
>     > +    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>     > +    {
>     > +        /* Map failed, populate ring page */
>     > +        rc = xc_domain_populate_physmap_exact(xch, domain_id,
>     > +                                              1, 0, 0, &ring_pfn);
>     > +        if ( rc != 0 )
>     > +        {
>     > +            PERROR("Failed to populate ring gfn\n");
>     > +            return -1;
>     > +        }
>     > +
>     > +        mmap_pfn = ring_pfn;
>     > +        ring_page = xc_map_foreign_batch(xch, domain_id,
>     > +                                        PROT_READ | PROT_WRITE,
>     &mmap_pfn, 1);
>     > +
>     > +        if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>     > +        {
>     > +            PERROR("Could not map the ring page\n");
>     > +            return -1;
>     > +        }
>     > +    }
>     > +
>     > +    /* Initialise Xen */
>     > +    rc = xc_mem_paging_enable(xch, domain_id, evtchn_port);
>     > +    if ( rc != 0 )
>     > +    {
>     > +        switch ( errno ) {
>     > +            case EBUSY:
>     > +                ERROR("mempaging is (or was) active on this
>     domain");
>     > +                break;
>     > +            case ENODEV:
>     > +                ERROR("mempaging requires Hardware Assisted
>     Paging");
>     > +                break;
>     > +            case EMLINK:
>     > +                ERROR("mempaging not supported while iommu
>     passthrough is enabled");
>     > +                break;
>     > +            case EXDEV:
>     > +                ERROR("mempaging not supported in a PoD guest");
>     > +                break;
>     > +            default:
>     > +                PERROR("mempaging: error initialising shared
>     page");
>     > +                break;
>     > +        }
>     > +        return -1;
>     > +    }
>     > +
>     > +    /* Open event channel */
>     > +    *xce_handle = xc_evtchn_open(NULL, 0);
>     > +    if ( *xce_handle == NULL )
>     > +    {
>     > +        PERROR("Failed to open event channel");
>     > +        return -1;
>     > +    }
>
>     This is inappropriate inside libxenguest.  The user of the library
>     possibly already has open evtchn handle.  While this is only
>     wasteful of
>     an fd under linux, I believe it will result in open() failing
>     under some
>     of the *BSDs
>
>     The correct way of doing this is to have the caller of
>     xc_mem_paging_ring_setup() provide their xce_handle, and require
>     them to
>     open one if they need to.
>
> I think this is heavy handed. I mean, the idea here is to relieve the
> caller from doing all the setup work.

calling xc_evtchn_open() is *not* something which should ever be
relieved from the caller.

>
> In fact, you could argue that a follow-up patch should encapsulate all
> the cleanup.
> And then consumers of this particular module call setup, teardown, use
> the intermediate result, no concerns.

I realise I am not a maintainer, so ultimately it is Ian/Ian you have to
convince in this matter.


If I were doing this, there would be two patches.  The first touches
libxc alone and introduces this setup function and a teardown function,
both written correctly and bugfree.  The second patch modifies xenpaging
to use the new setup and teardown functions.

As it currently stands, this is a halfbaked copy and paste of too much
setup code, moving into a library without its equivalent teardown, and a
promise of a fixup patch.

>
> LGTM, to be honest. 
>
>
>     > +
>     > +    /* Bind event notification */
>     > +    rc = xc_evtchn_bind_interdomain(*xce_handle, domain_id,
>     *evtchn_port);
>     > +    if ( rc < 0 )
>     > +    {
>     > +        PERROR("Failed to bind event channel");
>     > +        return -1;
>     > +    }
>     > +    *port = rc;
>     > +
>     > +    /* Initialise ring */
>     > +    SHARED_RING_INIT((mem_event_sring_t *)ring_page);
>     > +    BACK_RING_INIT(back_ring, (mem_event_sring_t *)ring_page,
>     PAGE_SIZE);
>     > +
>     > +    /* Now that the ring is set, remove it from the guest's
>     physmap */
>     > +    if ( xc_domain_decrease_reservation_exact(xch, domain_id,
>     1, 0, &ring_pfn) )
>     > +    {
>     > +        PERROR("Failed to remove ring from guest physmap");
>     > +        return -1;
>     > +    }
>
>     There is a race condition here where the guest can play with the
>     post-initialised ring state.
>
> Well-known, not the place to fix it here.

I don't know how exactly the security policy applies here.  My
understanding is that the hypervisor side paging code has been declared
stable and therefore subject to XSAs.

The libxc codes is, is far as I can tell, just thin wrappers around the
hypercalls.  At the moment, the xenpaging utility is just example code,
so is probably fine from an XSA point of view.

However, moving code from xenpaging into libxc changes this split.


Speaking now with my XenServer security hat on,

Irrespective of whether it would technically be declared an XSA or not,
it is stupid to knowingly put a latent security bug in a library.  The
best case is that the development tree has a security bug in it for a
while.  The worst case is that you forget to fix it before the next Xen
release and everyone in the world picks up the security bug as part of
their stable code.

So no - this is absolutely the place to fix it.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 18567 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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