| Message ID | 20221026102018.4144-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | XSA-409 fixes | expand |
On Wed, Oct 26, 2022 at 6:21 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> > --- > diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c > index 2abaab439c4f..f93b221f1c1f 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_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem); Should this be xc_*set*_p2m_mempool_size? Regards, Jason
On 26/10/2022 14:22, Jason Andryuk wrote: > On Wed, Oct 26, 2022 at 6:21 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >> --- >> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c >> index 2abaab439c4f..f93b221f1c1f 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_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem); > Should this be xc_*set*_p2m_mempool_size? Hmm, yes it should be. And the reason this doesn't break any tests is because all examples in CI match the default that Xen that sets. ~Andrew
On Wed, 26 Oct 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. Genuine question: I can see this patch removes the implementation of XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both ARM and x86) to the new hypercall. Why keep the old hypercall (XEN_DOMCTL_shadow_op and XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? Is that because it was only recently implemented? And not actually present in any past Xen release? If so, please add a note about this in the commit message. Also, if that is the case, I think this patch series should go in 4.17. If it is too late to get it in before the release, then we should backport it to 4.17 as soon as possible. That's because ideally we want to keep the hypercall interface changes down to a minimum. > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Xen Security Team <security@xen.org> > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Henry Wang <Henry.Wang@arm.com> > CC: Anthony PERARD <anthony.perard@citrix.com> > --- > 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..f93b221f1c1f 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_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem); > + if (r) { > + LOGED(ERROR, domid, > + "Failed to set p2m pool 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 4607cde6f0b8..92b678cf0d09 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_p2m_mempool_size(struct domain *d, uint64_t *size) > { > -- > 2.11.0 >
On 16/11/2022 01:37, Stefano Stabellini wrote: > On Wed, 26 Oct 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. > Genuine question: I can see this patch removes the implementation of > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both > ARM and x86) to the new hypercall. > > Why keep the old hypercall (XEN_DOMCTL_shadow_op and > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? > > Is that because it was only recently implemented? And not actually > present in any past Xen release? > > If so, please add a note about this in the commit message. Also, if that > is the case, I think this patch series should go in 4.17. If it is too > late to get it in before the release, then we should backport it to 4.17 > as soon as possible. That's because ideally we want to keep the > hypercall interface changes down to a minimum. On ARM, the hypercall has existed for a little over 4 weeks, and isn't in any released version of Xen (yet). On x86, the hypercall has existed for more than a decade, and has known out-of-tree users. It needs to be deprecated properly, which in this case means "phased out in the 4.18 cycle once known callers have been adapted to the new hypercall". ~Andrew
On Wed, 16 Nov 2022, Andrew Cooper wrote: > On 16/11/2022 01:37, Stefano Stabellini wrote: > > On Wed, 26 Oct 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. > > Genuine question: I can see this patch removes the implementation of > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both > > ARM and x86) to the new hypercall. > > > > Why keep the old hypercall (XEN_DOMCTL_shadow_op and > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? > > > > Is that because it was only recently implemented? And not actually > > present in any past Xen release? > > > > If so, please add a note about this in the commit message. Also, if that > > is the case, I think this patch series should go in 4.17. If it is too > > late to get it in before the release, then we should backport it to 4.17 > > as soon as possible. That's because ideally we want to keep the > > hypercall interface changes down to a minimum. > > On ARM, the hypercall has existed for a little over 4 weeks, and isn't > in any released version of Xen (yet). > > On x86, the hypercall has existed for more than a decade, and has known > out-of-tree users. It needs to be deprecated properly, which in this > case means "phased out in the 4.18 cycle once known callers have been > adapted to the new hypercall". Understoon. Then I am in favor of getting all 4 patches in 4.17, either before the release or via backports.
Hi Andrew and Stefano, Thanks for pushing things forward! > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Subject: Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; > use p2m mempool hypercalls > > On Wed, 16 Nov 2022, Andrew Cooper wrote: > > On 16/11/2022 01:37, Stefano Stabellini wrote: > > > On Wed, 26 Oct 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. > > > Genuine question: I can see this patch removes the implementation of > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches > libxl (both > > > ARM and x86) to the new hypercall. > > > > > > Why keep the old hypercall (XEN_DOMCTL_shadow_op and > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 > (not on ARM)? > > > > > > Is that because it was only recently implemented? And not actually > > > present in any past Xen release? > > > > > > If so, please add a note about this in the commit message. Also, if that > > > is the case, I think this patch series should go in 4.17. If it is too > > > late to get it in before the release, then we should backport it to 4.17 > > > as soon as possible. That's because ideally we want to keep the > > > hypercall interface changes down to a minimum. > > > > On ARM, the hypercall has existed for a little over 4 weeks, and isn't > > in any released version of Xen (yet). > > > > On x86, the hypercall has existed for more than a decade, and has known > > out-of-tree users. It needs to be deprecated properly, which in this > > case means "phased out in the 4.18 cycle once known callers have been > > adapted to the new hypercall". > > Understoon. Then I am in favor of getting all 4 patches in 4.17, either > before the release or via backports. Sorry - today it took me a little bit longer to get the office, so hopefully I still jumped into discussion on time. About this series, I don't have strong objection to taking all 4 patches, so if this series can have proper review/agreements by this weekend, feel free to add my release-ack for the patches. However, if we cannot sort out all 4 patches, I think at least patch #4 should go into 4.17 (with a commit message adjustment). The patch #4 already has proper tags from Arm maintainer and me. Kind regards, Henry
On 16.11.2022 03:00, Stefano Stabellini wrote: > On Wed, 16 Nov 2022, Andrew Cooper wrote: >> On 16/11/2022 01:37, Stefano Stabellini wrote: >>> On Wed, 26 Oct 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. >>> Genuine question: I can see this patch removes the implementation of >>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both >>> ARM and x86) to the new hypercall. >>> >>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and >>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? >>> >>> Is that because it was only recently implemented? And not actually >>> present in any past Xen release? >>> >>> If so, please add a note about this in the commit message. Also, if that >>> is the case, I think this patch series should go in 4.17. If it is too >>> late to get it in before the release, then we should backport it to 4.17 >>> as soon as possible. That's because ideally we want to keep the >>> hypercall interface changes down to a minimum. >> >> On ARM, the hypercall has existed for a little over 4 weeks, and isn't >> in any released version of Xen (yet). >> >> On x86, the hypercall has existed for more than a decade, and has known >> out-of-tree users. It needs to be deprecated properly, which in this >> case means "phased out in the 4.18 cycle once known callers have been >> adapted to the new hypercall". > > Understoon. Then I am in favor of getting all 4 patches in 4.17, either > before the release or via backports. Removing something from the domctl interface generally requires bumping the interface version, so some extra care may need applying if such an interface change was to be backported to any stable branch. Jan
On 16/11/2022 08:30, Jan Beulich wrote: > On 16.11.2022 03:00, Stefano Stabellini wrote: >> On Wed, 16 Nov 2022, Andrew Cooper wrote: >>> On 16/11/2022 01:37, Stefano Stabellini wrote: >>>> On Wed, 26 Oct 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. >>>> Genuine question: I can see this patch removes the implementation of >>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both >>>> ARM and x86) to the new hypercall. >>>> >>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and >>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? >>>> >>>> Is that because it was only recently implemented? And not actually >>>> present in any past Xen release? >>>> >>>> If so, please add a note about this in the commit message. Also, if that >>>> is the case, I think this patch series should go in 4.17. If it is too >>>> late to get it in before the release, then we should backport it to 4.17 >>>> as soon as possible. That's because ideally we want to keep the >>>> hypercall interface changes down to a minimum. >>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't >>> in any released version of Xen (yet). >>> >>> On x86, the hypercall has existed for more than a decade, and has known >>> out-of-tree users. It needs to be deprecated properly, which in this >>> case means "phased out in the 4.18 cycle once known callers have been >>> adapted to the new hypercall". >> Understoon. Then I am in favor of getting all 4 patches in 4.17, either >> before the release or via backports. > Removing something from the domctl interface generally requires bumping > the interface version, so some extra care may need applying if such an > interface change was to be backported to any stable branch. To be clear, I have no plans to remove the x86 "older" interface in this patch series. It will definitely break out of tree users. In the 4.18 timeframe, we can see about retiring the older hypercalls, but as a non-backportable change. ~Andrew
On Wed, 16 Nov 2022, Andrew Cooper wrote: > On 16/11/2022 08:30, Jan Beulich wrote: > > On 16.11.2022 03:00, Stefano Stabellini wrote: > >> On Wed, 16 Nov 2022, Andrew Cooper wrote: > >>> On 16/11/2022 01:37, Stefano Stabellini wrote: > >>>> On Wed, 26 Oct 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. > >>>> Genuine question: I can see this patch removes the implementation of > >>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both > >>>> ARM and x86) to the new hypercall. > >>>> > >>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and > >>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? > >>>> > >>>> Is that because it was only recently implemented? And not actually > >>>> present in any past Xen release? > >>>> > >>>> If so, please add a note about this in the commit message. Also, if that > >>>> is the case, I think this patch series should go in 4.17. If it is too > >>>> late to get it in before the release, then we should backport it to 4.17 > >>>> as soon as possible. That's because ideally we want to keep the > >>>> hypercall interface changes down to a minimum. > >>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't > >>> in any released version of Xen (yet). > >>> > >>> On x86, the hypercall has existed for more than a decade, and has known > >>> out-of-tree users. It needs to be deprecated properly, which in this > >>> case means "phased out in the 4.18 cycle once known callers have been > >>> adapted to the new hypercall". > >> Understoon. Then I am in favor of getting all 4 patches in 4.17, either > >> before the release or via backports. > > Removing something from the domctl interface generally requires bumping > > the interface version, so some extra care may need applying if such an > > interface change was to be backported to any stable branch. > > To be clear, I have no plans to remove the x86 "older" interface in this > patch series. It will definitely break out of tree users. > > In the 4.18 timeframe, we can see about retiring the older hypercalls, > but as a non-backportable change. For ARM, given that XEN_DOMCTL_shadow_op has not been enabled for long, maybe we can get away without bumping the interface version?
Hi, On 16/11/2022 23:44, Stefano Stabellini wrote: > On Wed, 16 Nov 2022, Andrew Cooper wrote: >> On 16/11/2022 08:30, Jan Beulich wrote: >>> On 16.11.2022 03:00, Stefano Stabellini wrote: >>>> On Wed, 16 Nov 2022, Andrew Cooper wrote: >>>>> On 16/11/2022 01:37, Stefano Stabellini wrote: >>>>>> On Wed, 26 Oct 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. >>>>>> Genuine question: I can see this patch removes the implementation of >>>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both >>>>>> ARM and x86) to the new hypercall. >>>>>> >>>>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and >>>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? >>>>>> >>>>>> Is that because it was only recently implemented? And not actually >>>>>> present in any past Xen release? >>>>>> >>>>>> If so, please add a note about this in the commit message. Also, if that >>>>>> is the case, I think this patch series should go in 4.17. If it is too >>>>>> late to get it in before the release, then we should backport it to 4.17 >>>>>> as soon as possible. That's because ideally we want to keep the >>>>>> hypercall interface changes down to a minimum. >>>>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't >>>>> in any released version of Xen (yet). >>>>> >>>>> On x86, the hypercall has existed for more than a decade, and has known >>>>> out-of-tree users. It needs to be deprecated properly, which in this >>>>> case means "phased out in the 4.18 cycle once known callers have been >>>>> adapted to the new hypercall". >>>> Understoon. Then I am in favor of getting all 4 patches in 4.17, either >>>> before the release or via backports. >>> Removing something from the domctl interface generally requires bumping >>> the interface version, so some extra care may need applying if such an >>> interface change was to be backported to any stable branch. >> >> To be clear, I have no plans to remove the x86 "older" interface in this >> patch series. It will definitely break out of tree users. >> >> In the 4.18 timeframe, we can see about retiring the older hypercalls, >> but as a non-backportable change. > > For ARM, given that XEN_DOMCTL_shadow_op has not been enabled for long, > maybe we can get away without bumping the interface version? IMHO how long it was out doesn't matter. Once we do a release, we should avoid changing the interface in minor version. This is because a user may start to rely on it and we don't want to break them for minor releases. Cheers,
On Wed, 16 Nov 2022, Julien Grall wrote: > On 16/11/2022 23:44, Stefano Stabellini wrote: > > On Wed, 16 Nov 2022, Andrew Cooper wrote: > > > On 16/11/2022 08:30, Jan Beulich wrote: > > > > On 16.11.2022 03:00, Stefano Stabellini wrote: > > > > > On Wed, 16 Nov 2022, Andrew Cooper wrote: > > > > > > On 16/11/2022 01:37, Stefano Stabellini wrote: > > > > > > > On Wed, 26 Oct 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. > > > > > > > Genuine question: I can see this patch removes the implementation > > > > > > > of > > > > > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl > > > > > > > (both > > > > > > > ARM and x86) to the new hypercall. > > > > > > > > > > > > > > Why keep the old hypercall (XEN_DOMCTL_shadow_op and > > > > > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on > > > > > > > ARM)? > > > > > > > > > > > > > > Is that because it was only recently implemented? And not actually > > > > > > > present in any past Xen release? > > > > > > > > > > > > > > If so, please add a note about this in the commit message. Also, > > > > > > > if that > > > > > > > is the case, I think this patch series should go in 4.17. If it is > > > > > > > too > > > > > > > late to get it in before the release, then we should backport it > > > > > > > to 4.17 > > > > > > > as soon as possible. That's because ideally we want to keep the > > > > > > > hypercall interface changes down to a minimum. > > > > > > On ARM, the hypercall has existed for a little over 4 weeks, and > > > > > > isn't > > > > > > in any released version of Xen (yet). > > > > > > > > > > > > On x86, the hypercall has existed for more than a decade, and has > > > > > > known > > > > > > out-of-tree users. It needs to be deprecated properly, which in > > > > > > this > > > > > > case means "phased out in the 4.18 cycle once known callers have > > > > > > been > > > > > > adapted to the new hypercall". > > > > > Understoon. Then I am in favor of getting all 4 patches in 4.17, > > > > > either > > > > > before the release or via backports. > > > > Removing something from the domctl interface generally requires bumping > > > > the interface version, so some extra care may need applying if such an > > > > interface change was to be backported to any stable branch. > > > > > > To be clear, I have no plans to remove the x86 "older" interface in this > > > patch series. It will definitely break out of tree users. > > > > > > In the 4.18 timeframe, we can see about retiring the older hypercalls, > > > but as a non-backportable change. > > > > For ARM, given that XEN_DOMCTL_shadow_op has not been enabled for long, > > maybe we can get away without bumping the interface version? > > IMHO how long it was out doesn't matter. Once we do a release, we should avoid > changing the interface in minor version. > > This is because a user may start to rely on it and we don't want to break them > for minor releases. We haven't released 4.17 yet, so I take you are referring to a stable minor release, right?
On 17.11.2022 00:41, Andrew Cooper wrote: > On 16/11/2022 08:30, Jan Beulich wrote: >> On 16.11.2022 03:00, Stefano Stabellini wrote: >>> On Wed, 16 Nov 2022, Andrew Cooper wrote: >>>> On 16/11/2022 01:37, Stefano Stabellini wrote: >>>>> On Wed, 26 Oct 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. >>>>> Genuine question: I can see this patch removes the implementation of >>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both >>>>> ARM and x86) to the new hypercall. >>>>> >>>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and >>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)? >>>>> >>>>> Is that because it was only recently implemented? And not actually >>>>> present in any past Xen release? >>>>> >>>>> If so, please add a note about this in the commit message. Also, if that >>>>> is the case, I think this patch series should go in 4.17. If it is too >>>>> late to get it in before the release, then we should backport it to 4.17 >>>>> as soon as possible. That's because ideally we want to keep the >>>>> hypercall interface changes down to a minimum. >>>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't >>>> in any released version of Xen (yet). >>>> >>>> On x86, the hypercall has existed for more than a decade, and has known >>>> out-of-tree users. It needs to be deprecated properly, which in this >>>> case means "phased out in the 4.18 cycle once known callers have been >>>> adapted to the new hypercall". >>> Understoon. Then I am in favor of getting all 4 patches in 4.17, either >>> before the release or via backports. >> Removing something from the domctl interface generally requires bumping >> the interface version, so some extra care may need applying if such an >> interface change was to be backported to any stable branch. > > To be clear, I have no plans to remove the x86 "older" interface in this > patch series. It will definitely break out of tree users. > > In the 4.18 timeframe, we can see about retiring the older hypercalls, > but as a non-backportable change. Sure, but I was referring to the (pretty new) Arm incarnation thereof. Jan
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..f93b221f1c1f 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_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem); + if (r) { + LOGED(ERROR, domid, + "Failed to set p2m pool 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 4607cde6f0b8..92b678cf0d09 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_p2m_mempool_size(struct domain *d, uint64_t *size) {
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@citrix.com> --- CC: Xen Security Team <security@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Henry Wang <Henry.Wang@arm.com> CC: Anthony PERARD <anthony.perard@citrix.com> --- 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(-)