[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls



On Thu, 17 Nov 2022, Andrew Cooper wrote:
> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
> 
> First of all, with ARM borrowing x86's implementation, the logic to set the
> pool size should have been common, not duplicated.  Introduce
> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
> libxl/xl can reasonably function without the ability to query the pool size...
> 
> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
> replaced with a working and unit tested interface.
> 
> This is part of XSA-409 / CVE-2022-33747.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Henry Wang <Henry.Wang@xxxxxxx>
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> v2:
>  * s/p2m/paging/
>  * Fix get/set typo in libxl__domain_set_p2m_pool_size()
> ---
>  tools/libs/light/libxl_arm.c      | 14 +----------
>  tools/libs/light/libxl_dom.c      | 19 ++++++++++++++
>  tools/libs/light/libxl_internal.h |  3 +++
>  tools/libs/light/libxl_x86.c      | 15 ++---------
>  xen/arch/arm/domctl.c             | 53 
> ---------------------------------------
>  xen/arch/arm/include/asm/p2m.h    |  1 -
>  xen/arch/arm/p2m.c                |  8 ------
>  7 files changed, 25 insertions(+), 88 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2a5e93c28403..2f5615263543 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -209,19 +209,7 @@ int libxl__arch_domain_create(libxl__gc *gc,
>                                libxl__domain_build_state *state,
>                                uint32_t domid)
>  {
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
> -    unsigned int shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb, 
> 1024);
> -
> -    int r = xc_shadow_control(ctx->xch, domid,
> -                              XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> -                              &shadow_mb, 0);
> -    if (r) {
> -        LOGED(ERROR, domid,
> -              "Failed to set %u MiB shadow allocation", shadow_mb);
> -        return ERROR_FAIL;
> -    }
> -
> -    return 0;
> +    return libxl__domain_set_p2m_pool_size(gc, d_config, domid);
>  }
>  
>  int libxl__arch_extra_memory(libxl__gc *gc,
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 2abaab439c4f..f8f7b7e81837 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -1448,6 +1448,25 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t 
> domid,
>      return rc;
>  }
>  
> +int libxl__domain_set_p2m_pool_size(
> +    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    uint64_t shadow_mem;
> +
> +    shadow_mem = d_config->b_info.shadow_memkb;
> +    shadow_mem <<= 10;
> +
> +    int r = xc_set_paging_mempool_size(ctx->xch, domid, shadow_mem);
> +    if (r) {
> +        LOGED(ERROR, domid,
> +              "Failed to set paging mempool size to %"PRIu64"kB", 
> shadow_mem);
> +        return ERROR_FAIL;
> +    }
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/light/libxl_internal.h 
> b/tools/libs/light/libxl_internal.h
> index cb9e8b3b8b5a..f31164bc6c0d 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -4864,6 +4864,9 @@ int libxl__is_domid_recent(libxl__gc *gc, uint32_t 
> domid, bool *recent);
>  /* os-specific implementation of setresuid() */
>  int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);
>  
> +_hidden int libxl__domain_set_p2m_pool_size(
> +    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid);
> +
>  #endif
>  
>  /*
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 7c5ee74443e5..99aba51d05df 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -538,20 +538,9 @@ int libxl__arch_domain_create(libxl__gc *gc,
>          xc_domain_set_time_offset(ctx->xch, domid, rtc_timeoffset);
>  
>      if (d_config->b_info.type != LIBXL_DOMAIN_TYPE_PV) {
> -        unsigned int shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb,
> -                                             1024);
> -        int r = xc_shadow_control(ctx->xch, domid,
> -                                  XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> -                                  &shadow_mb, 0);
> -
> -        if (r) {
> -            LOGED(ERROR, domid,
> -                  "Failed to set %u MiB %s allocation",
> -                  shadow_mb,
> -                  libxl_defbool_val(d_config->c_info.hap) ? "HAP" : 
> "shadow");
> -            ret = ERROR_FAIL;
> +        ret = libxl__domain_set_p2m_pool_size(gc, d_config, domid);
> +        if (ret)
>              goto out;
> -        }
>      }
>  
>      if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index c8fdeb124084..1baf25c3d98b 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -47,64 +47,11 @@ static int handle_vuart_init(struct domain *d,
>      return rc;
>  }
>  
> -static long p2m_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
> -                       XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> -{
> -    long rc;
> -    bool preempted = false;
> -
> -    if ( unlikely(d == current->domain) )
> -    {
> -        printk(XENLOG_ERR "Tried to do a p2m domctl op on itself.\n");
> -        return -EINVAL;
> -    }
> -
> -    if ( unlikely(d->is_dying) )
> -    {
> -        printk(XENLOG_ERR "Tried to do a p2m domctl op on dying domain %u\n",
> -               d->domain_id);
> -        return -EINVAL;
> -    }
> -
> -    switch ( sc->op )
> -    {
> -    case XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION:
> -    {
> -        /* Allow and handle preemption */
> -        spin_lock(&d->arch.paging.lock);
> -        rc = p2m_set_allocation(d, sc->mb << (20 - PAGE_SHIFT), &preempted);
> -        spin_unlock(&d->arch.paging.lock);
> -
> -        if ( preempted )
> -            /* Not finished. Set up to re-run the call. */
> -            rc = hypercall_create_continuation(__HYPERVISOR_domctl, "h",
> -                                               u_domctl);
> -        else
> -            /* Finished. Return the new allocation. */
> -            sc->mb = p2m_get_allocation(d);
> -
> -        return rc;
> -    }
> -    case XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION:
> -    {
> -        sc->mb = p2m_get_allocation(d);
> -        return 0;
> -    }
> -    default:
> -    {
> -        printk(XENLOG_ERR "Bad p2m domctl op %u\n", sc->op);
> -        return -EINVAL;
> -    }
> -    }
> -}
> -
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
>      switch ( domctl->cmd )
>      {
> -    case XEN_DOMCTL_shadow_op:
> -        return p2m_domctl(d, &domctl->u.shadow_op, u_domctl);
>      case XEN_DOMCTL_cacheflush:
>      {
>          gfn_t s = _gfn(domctl->u.cacheflush.start_pfn);
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index c8f14d13c2c5..91df922e1c9f 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -222,7 +222,6 @@ void p2m_restore_state(struct vcpu *n);
>  /* Print debugging/statistial info about a domain's p2m */
>  void p2m_dump_info(struct domain *d);
>  
> -unsigned int p2m_get_allocation(struct domain *d);
>  int p2m_set_allocation(struct domain *d, unsigned long pages, bool 
> *preempted);
>  int p2m_teardown_allocation(struct domain *d);
>  
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8c1972e58227..b2f7e8d804aa 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -92,14 +92,6 @@ static void p2m_free_page(struct domain *d, struct 
> page_info *pg)
>      spin_unlock(&d->arch.paging.lock);
>  }
>  
> -/* Return the size of the pool, rounded up to the nearest MB */
> -unsigned int p2m_get_allocation(struct domain *d)
> -{
> -    unsigned long nr_pages = ACCESS_ONCE(d->arch.paging.p2m_total_pages);
> -
> -    return ROUNDUP(nr_pages, 1 << (20 - PAGE_SHIFT)) >> (20 - PAGE_SHIFT);
> -}
> -
>  /* Return the size of the pool, in bytes. */
>  int arch_get_paging_mempool_size(struct domain *d, uint64_t *size)
>  {
> -- 
> 2.11.0
> 

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.