Re: [Xen-devel] Backport request "libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap." (Was: Re: [Bug report] Security issue in "xl vcpu-set")

[Luwei Cheng <chengluwei@xxxxxxxxx>]

On Fri, Jun 5, 2015 at 7:13 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:

On Thu, 2015-06-04 at 15:54 +0100, Ian Campbell wrote:
> (redirecting to xen-devel as I originally intended)
>
> On Wed, 2015-06-03 at 13:02 +0800, lwcheng@xxxxxxxxx wrote:
> > Hi,
> >
> > Wonder if there is any follow-ups from the relevant developers:
> > (1) are you able to reproduce the "spinning" behavior of "xl vcpu-set"?
>
> I am with Xen 4.5, but not with xen-unstable AFAICT.
>
> > (2) if yes, can you confirm that it is due to looping with
> > "retry_transaction"?
>
> I don't think so. You are _supposed_ to retry failed transactions in
> this way, it's how they work.
>
> The issue is that the transaction is failing repeatedly in such a
> manner. This might be due to a lack of error checking within the loop,
> or it could possibly be a bug in the xenstore daemon.

The real issue is that the loop terminator (info.vcpu_max_id) is -1, so
we are just looping trying to set 4 billion or so CPUs online.

Oh yes, you are completely right. xenstore access log shows it isÂ

indeed setting a huge amount of assumed online CPUs.

Â

Konrad's change below added an error check for this case, it should
probably be backported.

Given the operation can be interrupted with Ctrl-C I'm not sure there is
any security impact.

Some third-part management tools might be built directly above xl.

Perhaps they can not rely on "Ctrl-C"..

Best,

Luwei

Â

Ian.


commit d83bf9d224eeb5b73b93c2703f7dba4473cfa89c
Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date:Â ÂFri Apr 3 16:02:29 2015 -0400

  libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

  There is no sense in trying to online (or offline) CPUs when the size of
  cpumap is greater than the maximum number of VCPUs the guest can go to.

  As such fail the operation if the count of CPUs to online is greater
  than what the guest started with. For the offline case we do not
  check (as the bits are unset in the cpumap) and let it go through.

  We coalesce some of the underlying libxl_set_vcpuonline code
  together which was duplicated in QMP and XenStore codepaths.

  Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
  Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e6eebcc..debb20c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5445,27 +5445,19 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
Â}

Âstatic int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
-Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlibxl_bitmap *cpumap)
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlibxl_bitmap *cpumap,
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âconst libxl_dominfo *info)
Â{
-Â Â libxl_dominfo info;
  Âchar *dompath;
  Âxs_transaction_t t;
-Â Â int i, rc;
-
-Â Â libxl_dominfo_init(&info);
+Â Â int i, rc = ERROR_FAIL;

-Â Â rc = libxl_domain_info(CTX, &info, domid);
-Â Â if (rc < 0) {
-Â Â Â Â LOGE(ERROR, "getting domain info list");
-Â Â Â Â goto out;
-Â Â }
-Â Â rc = ERROR_FAIL;
  Âif (!(dompath = libxl__xs_get_dompath(gc, domid)))
    Âgoto out;

Âretry_transaction:
  Ât = xs_transaction_start(CTX->xsh);
-Â Â for (i = 0; i <= info.vcpu_max_id; i++)
+Â Â for (i = 0; i <= info->vcpu_max_id; i++)
    Âlibxl__xs_write(gc, t,
            libxl__sprintf(gc, "%s/cpu/%u/availability", dompath, i),
            "%s", libxl_bitmap_test(cpumap, i) ? "online" : "offline");
@@ -5475,25 +5467,16 @@ retry_transaction:
  Â} else
    Ârc = 0;
Âout:
-Â Â libxl_dominfo_dispose(&info);
  Âreturn rc;
Â}

Âstatic int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
-Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlibxl_bitmap *cpumap)
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlibxl_bitmap *cpumap,
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âconst libxl_dominfo *info)
Â{
-Â Â libxl_dominfo info;
-Â Â int i, rc;
-
-Â Â libxl_dominfo_init(&info);
+Â Â int i;

-Â Â rc = libxl_domain_info(CTX, &info, domid);
-Â Â if (rc < 0) {
-Â Â Â Â LOGE(ERROR, "getting domain info list");
-Â Â Â Â libxl_dominfo_dispose(&info);
-Â Â Â Â return rc;
-Â Â }
-Â Â for (i = 0; i <= info.vcpu_max_id; i++) {
+Â Â for (i = 0; i <= info->vcpu_max_id; i++) {
    Âif (libxl_bitmap_test(cpumap, i)) {
      Â/* Return value is ignore because it does not tell anything useful
       * on the completion of the command.
@@ -5503,33 +5486,53 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
      Âlibxl__qmp_cpu_add(gc, domid, i);
    Â}
  Â}
-Â Â libxl_dominfo_dispose(&info);
  Âreturn 0;
Â}

Âint libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
Â{
  ÂGC_INIT(ctx);
-Â Â int rc;
+Â Â int rc, maxcpus;
+Â Â libxl_dominfo info;
+
+Â Â libxl_dominfo_init(&info);
+
+Â Â rc = libxl_domain_info(CTX, &info, domid);
+Â Â if (rc < 0) {
+Â Â Â Â LOGE(ERROR, "getting domain info list");
+Â Â Â Â goto out;
+Â Â }
+
+Â Â maxcpus = libxl_bitmap_count_set(cpumap);
+Â Â if (maxcpus > info.vcpu_max_id + 1)
+Â Â {
+Â Â Â Â LOGE(ERROR, "Requested %d VCPUs, however maxcpus is %d!",
+Â Â Â Â Â Â Âmaxcpus, info.vcpu_max_id + 1);
+Â Â Â Â rc = ERROR_FAIL;
+Â Â Â Â goto out;
+Â Â }
+
  Âswitch (libxl__domain_type(gc, domid)) {
  Âcase LIBXL_DOMAIN_TYPE_HVM:
    Âswitch (libxl__device_model_version_running(gc, domid)) {
    Âcase LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
-Â Â Â Â Â Â rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
+Â Â Â Â Â Â rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
      Âbreak;
    Âcase LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-Â Â Â Â Â Â rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap);
+Â Â Â Â Â Â rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap, &info);
      Âbreak;
    Âdefault:
      Ârc = ERROR_INVAL;
    Â}
    Âbreak;
  Âcase LIBXL_DOMAIN_TYPE_PV:
-Â Â Â Â rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
+Â Â Â Â rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
    Âbreak;
  Âdefault:
    Ârc = ERROR_INVAL;
  Â}
+out:
+Â Â libxl_dominfo_dispose(&info);
  ÂGC_FREE;
  Âreturn rc;
Â}

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