Re: [Bug report] Security issue in "xl vcpu-set"

Re: [Bug report] Security issue in "xl vcpu-set"

[Ian Campbell]

(redirecting to xen-devel as I originally intended)

On Wed, 2015-06-03 at 13:02 +0800, lwcheng@cs.hku.hk 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.

Ian.

> 
> Best,
> Luwei
> 
> 
> Quoting lwcheng@cs.hku.hk:
> 
> > Hi Ian,
> >
> > Thanks for your reply. Please read my inline reply to your questions.
> >
> > Quoting Ian Campbell <ian.campbell@citrix.com>:
> >> Since this was copied to xen-api@ it is now public, so redirecting to
> >> the correct list (xen-devel@). I kept xen-api since oxenstored might be
> >> involved. I dropped Vincent since he is no longer involved in libxl
> >> development.
> >>
> >> On Fri, 2015-05-29 at 13:44 +0800, lwcheng@cs.hku.hk wrote:
> >>> Hi,
> >>>
> >>> "xl vcpu-set" is commonly used to hotplug/unhotplug vCPUs of an SMP VM.
> >>> However, the current implementation of this command makes the driver
> >>> domain vulnerable to denial-of-service attack: in certain cases,  
> >>> this command
> >>> consumes too many CPU cycles in dom0, adversely affecting dom0's other
> >>> tasks (e.g., IO processing, monitoring, etc.)
> >>>
> >>> [An illustrative example]
> >>> Say, with a Linux PV guest called "vm01", when vm01 just boots or reboots
> >>> (e.g., in its grub period)
> >>
> >> Do you mean pygrub or pvgrub here?
> >
> > My VM uses pygrub: Xen-4.5.0 + Linux 3.14.35 (for both dom0 and domU).
> >
> >>
> >>> , if dom0 issues "xl vcpu-set vm01 xxx" at this
> >>> moment, the following will happen:
> >>> (1) "xl vcpu-set" hangs, until vm01 has loaded its kernel successfully.
> >>> (2) in dom0, "oxenstored" consumes 100% of a single core.
> >>
> >> It's not clear to me why this should relate to the status of the guest,
> >> AFAIK there is no reason for a xenstore transaction to be affected by
> >> whether or not the guest has loaded its kernel.
> >>
> >> Certainly if it is spinning forever there is a bug somewhere, but I
> >> don't think it relates to the use of a transaction in this way.
> >
> > You may check /var/log/xenstored-access.log: when "xl vcpu-set" hangs,
> > xenstore keeps writing to "/local/domain/xx/cpu/xx/availability",
> > indicating that it is looping in retry_transaction.
> >
> >
> >>
> >> Ian.
> >>
> >>> So, if a malicious guest purposely stays in its grub period (or other
> >>> purposely designed phases, as long as it does not load its kernel),
> >>> "xl vcpu-set" will hang _forever_, and dom0 has to sacrifice one core.
> >>>
> >>> [Affected versions]
> >>> This problem has been there since libxl was introduced in Xen 4.1 release.
> >>>
> >>> [Implementation problem]
> >>> "xl vcpu-set" involves a "loop" as follows (retry_transaction):
> >>> --------------
> >>> static int libxl__set_vcpuonline_xenstore(...)
> >>> {
> >>> ... ...
> >>> retry_transaction:
> >>>     t = xs_transaction_start(CTX->xsh);
> >>>     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");
> >>>     if (!xs_transaction_end(CTX->xsh, t, 0)) {
> >>>         if (errno == EAGAIN)
> >>>             goto retry_transaction;
> >>>     }
> >>> ... ...
> >>> }
> >>> --------------
> >>>
> >>> [Possible solution]
> >>> In principle, the correctness of "xl vcpu-set" should _not_ depend on any
> >>> guest's behaviors.
> >>> One possible fix might be like this: if xs_transaction_end does  
> >>> not succeed,
> >>> either ignore it or retry for a pre-defined timeout period (rather
> >>> than forever).
> >>>
> >>> [Suggestions]
> >>> I find that the loop method like "retry_transaction" is used at  
> >>> several places
> >>> in libxl. You probably want to revisit the potential negative effect
> >>> it brings.
> >>>
> >>> Please take a look and help confirm my reported bug. Thanks.
> >>>
> >>> (Cc'd to two authors listed in libxl.c)
> >>>
> >>> Luwei Cheng
> >>> Department of Computer Science
> >>> The University of Hong Kong
> >>
> >>
> >
> >
> >
> 
> 

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")

[Ian Campbell]

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@cs.hku.hk 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.

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.

Ian.


commit d83bf9d224eeb5b73b93c2703f7dba4473cfa89c
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
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@oracle.com>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>

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;
 }

Re: 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]

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

On Fri, Jun 5, 2015 at 7:13 PM, Ian Campbell <ian.campbell@citrix.com>
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@cs.hku.hk 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@oracle.com>
> 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@oracle.com>
>     Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> 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;
>  }
>
>
>

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

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

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

Re: 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")

[Ian Jackson]

Luwei Cheng writes ("Re: 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")"):
> Some third-part management tools might be built directly above xl.
> Perhaps they can not rely on "Ctrl-C"..

In general callers of libxl will not be built to raise SIGINT.  For
example, if libvirt called this function in a way that triggers the
bug, there wouldn't be any reasonable way to recover control.

I'm afraid I'm still not clear about when the failure can be triggered
by an attacker.

Ian.

Re: 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")

[Ian Campbell]

On Mon, 2015-06-08 at 11:35 +0100, Ian Jackson wrote:
> Luwei Cheng writes ("Re: 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")"):
> > Some third-part management tools might be built directly above xl.
> > Perhaps they can not rely on "Ctrl-C"..
> 
> In general callers of libxl will not be built to raise SIGINT.  For
> example, if libvirt called this function in a way that triggers the
> bug, there wouldn't be any reasonable way to recover control.
> 
> I'm afraid I'm still not clear about when the failure can be triggered
> by an attacker.

I was able to reproduce by pressing a key at a pygrub prompt to drop to
a prompt and then leaving the guest in that state, where the domain
exists but does not yet have any vcpus etc.

Ian.

Re: 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")

[Ian Jackson]

Ian Campbell writes ("Re: 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")"):
> On Mon, 2015-06-08 at 11:35 +0100, Ian Jackson wrote:
> > I'm afraid I'm still not clear about when the failure can be triggered
> > by an attacker.
> 
> I was able to reproduce by pressing a key at a pygrub prompt to drop to
> a prompt and then leaving the guest in that state, where the domain
> exists but does not yet have any vcpus etc.

OK, then the fix should be backported.

The next question is whether there should be an advisory.

Ian.

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")

[Ian Jackson]

Ian Campbell writes ("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")"):
> commit d83bf9d224eeb5b73b93c2703f7dba4473cfa89c
> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date:   Fri Apr 3 16:02:29 2015 -0400
> 
>     libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.

Now backported to staging-4.5.  I fixed up the conflict, correctly I
think.

Ian.

commit 0d8cbcad03764e42ff2f0d224aff883c3734d782
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
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@oracle.com>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>
    (cherry picked from commit d83bf9d224eeb5b73b93c2703f7dba4473cfa89c)
    
    Conflicts:
    	tools/libxl/libxl.c
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1f4dce2..489d5f8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5487,25 +5487,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 = ERROR_FAIL;
 
-    libxl_dominfo_init(&info);
-
-    if (libxl_domain_info(CTX, &info, domid) < 0) {
-        LOGE(ERROR, "getting domain info list");
-        goto out;
-    }
     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");
@@ -5515,24 +5509,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;
 
-    libxl_dominfo_init(&info);
-
-    if (libxl_domain_info(CTX, &info, domid) < 0) {
-        LOGE(ERROR, "getting domain info list");
-        libxl_dominfo_dispose(&info);
-        return ERROR_FAIL;
-    }
-    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.
@@ -5542,33 +5528,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;
 }

Re: 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")

[Konrad Rzeszutek Wilk]

On Fri, Jun 12, 2015 at 01:02:47PM +0100, Ian Jackson wrote:
> Ian Campbell writes ("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")"):
> > commit d83bf9d224eeb5b73b93c2703f7dba4473cfa89c
> > Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date:   Fri Apr 3 16:02:29 2015 -0400
> > 
> >     libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap.
> 
> Now backported to staging-4.5.  I fixed up the conflict, correctly I
> think.

Yes, looks correctly.

> 
> Ian.
> 
> commit 0d8cbcad03764e42ff2f0d224aff883c3734d782
> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 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@oracle.com>
>     Acked-by: Ian Campbell <ian.campbell@citrix.com>
>     (cherry picked from commit d83bf9d224eeb5b73b93c2703f7dba4473cfa89c)
>     
>     Conflicts:
>     	tools/libxl/libxl.c
>     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 1f4dce2..489d5f8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5487,25 +5487,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 = ERROR_FAIL;
>  
> -    libxl_dominfo_init(&info);
> -
> -    if (libxl_domain_info(CTX, &info, domid) < 0) {
> -        LOGE(ERROR, "getting domain info list");
> -        goto out;
> -    }
>      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");
> @@ -5515,24 +5509,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;
>  
> -    libxl_dominfo_init(&info);
> -
> -    if (libxl_domain_info(CTX, &info, domid) < 0) {
> -        LOGE(ERROR, "getting domain info list");
> -        libxl_dominfo_dispose(&info);
> -        return ERROR_FAIL;
> -    }
> -    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.
> @@ -5542,33 +5528,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;
>  }