[Qemu-devel] [PATCH v4 0/7] pc: Ensure APIC ID limits before aborting or corrupting memory

[Qemu-devel] [PATCH v4 0/7] pc: Ensure APIC ID limits before aborting or corrupting memory

[Eduardo Habkost]

Changes v3 -> v4:
 * Commit message update on patch 5/7
 * Small comment change (s/should/shall/) on patch 6/7

Changes v2 -> v3:
 * Don't use MAX_CPUMASK_BITS on acpi-build.c, use ACPI_CPU_HOTPLUG_ID_LIMIT;
 * Rename MAX_CPUMASK_BITS to MAX_CPUS, and document it;
 * Use MAX_CPUS when checking max_cpus limit on vl.c.

Changes v1 -> v2:
 * None. v1 was tagged locally by mistake and never submitted to qemu-devel.

This series adds checks for APIC ID limits on initialization and CPU hotplug
code.  This fixes multiple issues:

1) Assertion failure when -smp parameter results in a too large APIC ID. e.g.:

    $ ./install/bin/qemu-system-x86_64 -S -smp 254,cores=17,threads=17,sockets=17,maxcpus=254 -nographic
    **
    ERROR:hw/acpi/cpu_hotplug.c:58:AcpiCpuHotplug_init: assertion failed: ((id / 8) < ACPI_GPE_PROC_LEN)
    Aborted (core dumped)

2) Memory corruption on AcpiCpuHotplug_add() when APIC ID is too large (similar
   to the case above, but on CPU hotplug).

Eduardo Habkost (7):
  acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro
  pc: Refuse CPU hotplug if the resulting APIC ID is too large
  acpi: Assert sts array limit on AcpiCpuHotplug_add()
  acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap
  pc: Refuse max_cpus if it results in too large APIC ID
  vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS
  vl.c: Use MAX_CPUS macro instead of hardcoded constant

 hw/acpi/cpu_hotplug.c              |  1 +
 hw/i386/acpi-build.c               |  4 ++--
 hw/i386/pc.c                       | 16 ++++++++++++++++
 include/hw/acpi/cpu_hotplug_defs.h |  8 ++++++++
 include/sysemu/sysemu.h            |  9 ++++++++-
 vl.c                               | 12 ++++++------
 6 files changed, 41 insertions(+), 9 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v4 1/7] acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro

[Eduardo Habkost]

The new macro will be helpful to allow us to detect too large SMP limits
before it is too late.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/acpi/cpu_hotplug_defs.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/hw/acpi/cpu_hotplug_defs.h b/include/hw/acpi/cpu_hotplug_defs.h
index 2725b50..9f33663 100644
--- a/include/hw/acpi/cpu_hotplug_defs.h
+++ b/include/hw/acpi/cpu_hotplug_defs.h
@@ -17,7 +17,15 @@
  * between C and ASL code.
  */
 #define ACPI_CPU_HOTPLUG_STATUS 4
+
+/* Limit for CPU arch IDs for CPU hotplug. All hotpluggable CPUs should
+ * have CPUClass.get_arch_id() < ACPI_CPU_HOTPLUG_ID_LIMIT.
+ */
+#define ACPI_CPU_HOTPLUG_ID_LIMIT 256
+
+/* 256 CPU IDs, 8 bits per entry: */
 #define ACPI_GPE_PROC_LEN 32
+
 #define ICH9_CPU_HOTPLUG_IO_BASE 0x0CD8
 #define PIIX4_CPU_HOTPLUG_IO_BASE 0xaf00
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 2/7] pc: Refuse CPU hotplug if the resulting APIC ID is too large

[Eduardo Habkost]

The ACPI CPU hotplug code requires APIC IDs to be smaller than
ACPI_CPU_HOTPLUG_ID_LIMIT, so enforce the limit before trying to hotplug
a new vCPU, returning an error instead of crashing.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e715a33..74cb4f9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -53,6 +53,7 @@
 #include "qemu/bitmap.h"
 #include "qemu/config-file.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/cpu_hotplug.h"
 #include "hw/cpu/icc_bus.h"
 #include "hw/boards.h"
 #include "hw/pci/pci_host.h"
@@ -974,6 +975,13 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
+    if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
+        error_setg(errp, "Unable to add CPU: %" PRIi64
+                   ", resulting APIC ID (%" PRIi64 ") is too large",
+                   id, apic_id);
+        return;
+    }
+
     icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
                                                  TYPE_ICC_BRIDGE, NULL));
     pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 3/7] acpi: Assert sts array limit on AcpiCpuHotplug_add()

[Eduardo Habkost]

AcpiCpuHotplug_add() can't handle vCPU arch IDs larger than
ACPI_CPU_HOTPLUG_ID_LIMIT. Instead of corrupting memory in case the vCPU
ID is too large, use g_assert() to ensure we are not over the limit.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/acpi/cpu_hotplug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 48928dc..2ad83a0 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -43,6 +43,7 @@ void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu)
 
     *gpe->sts = *gpe->sts | ACPI_CPU_HOTPLUG_STATUS;
     cpu_id = k->get_arch_id(CPU(cpu));
+    g_assert((cpu_id / 8) < ACPI_GPE_PROC_LEN);
     g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
 }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 4/7] acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap

[Eduardo Habkost]

MAX_CPUMASK_BITS is a limit for max_cpus and CPU indexes, not for APIC
IDs.

ACPI_CPU_HOTPLUG_ID_LIMIT is the right macro for the limit on APIC IDs
on the ACPI and CPU hotplug code.

There are no functional changes introduced by this patch, as
MAX_CPUMASK_BITS + 1 == 255 + 1 == 256 == ACPI_CPU_HOTPLUG_ID_LIMIT.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b667d31..749af1e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -52,7 +52,7 @@
 #include "qom/qom-qobject.h"
 
 typedef struct AcpiCpuInfo {
-    DECLARE_BITMAP(found_cpus, MAX_CPUMASK_BITS + 1);
+    DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
 } AcpiCpuInfo;
 
 typedef struct AcpiMcfgInfo {
@@ -117,7 +117,7 @@ int acpi_add_cpu_info(Object *o, void *opaque)
 
     if (object_dynamic_cast(o, TYPE_CPU)) {
         apic_id = object_property_get_int(o, "apic-id", NULL);
-        assert(apic_id <= MAX_CPUMASK_BITS);
+        assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
 
         set_bit(apic_id, cpu->found_cpus);
     }
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 5/7] pc: Refuse max_cpus if it results in too large APIC ID

[Eduardo Habkost]

This changes the PC initialization code to reject max_cpus if it results
in an APIC ID that's too large, instead of aborting or erroring out when
it is already too late.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
Changes v3 -> v4:
 * Commit message update: removed outdated comments about
   MAX_CPUMASK_BITS

Changes v2 -> v3:
 * No need to check against MAX_CPUMASK_BITS, as MAX_CPUMASK_BITS
   is used only for CPU-index-based bitmaps on NUMA code, not for APIC
   IDs.
---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 74cb4f9..14f0d91 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -992,6 +992,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     int i;
     X86CPU *cpu = NULL;
     Error *error = NULL;
+    unsigned long apic_id_limit;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -1003,6 +1004,13 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     }
     current_cpu_model = cpu_model;
 
+    apic_id_limit = pc_apic_id_limit(max_cpus);
+    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
+        error_report("max_cpus is too large. APIC ID of last CPU is %lu",
+                     apic_id_limit - 1);
+        exit(1);
+    }
+
     for (i = 0; i < smp_cpus; i++) {
         cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
                          icc_bridge, &error);
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 6/7] vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS

[Eduardo Habkost]

Also, document what the macro is really useful for.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
Changes v3 -> v4:
  * s/should/shall/ at the MAX_CPUS comment
---
 include/sysemu/sysemu.h |  9 ++++++++-
 vl.c                    | 10 +++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b90df9a..6bc1a85 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -133,7 +133,14 @@ extern uint8_t qemu_extra_params_fw[2];
 extern QEMUClockType rtc_clock;
 
 #define MAX_NODES 64
-#define MAX_CPUMASK_BITS 255
+
+/* The following shall be true for all CPUs:
+ *   cpu->cpu_index < max_cpus <= MAX_CPUS
+ *
+ * Note that cpu->get_arch_id() may be larger than MAX_CPUS.
+ */
+#define MAX_CPUS 255
+
 extern int nb_numa_nodes;
 extern uint64_t node_mem[MAX_NODES];
 extern unsigned long *node_cpumask[MAX_NODES];
diff --git a/vl.c b/vl.c
index bca5c95..64b38a5 100644
--- a/vl.c
+++ b/vl.c
@@ -1278,11 +1278,11 @@ static void numa_node_parse_cpus(int nodenr, const char *cpus)
         goto error;
     }
 
-    if (endvalue >= MAX_CPUMASK_BITS) {
-        endvalue = MAX_CPUMASK_BITS - 1;
+    if (endvalue >= MAX_CPUS) {
+        endvalue = MAX_CPUS - 1;
         fprintf(stderr,
             "qemu: NUMA: A max of %d VCPUs are supported\n",
-             MAX_CPUMASK_BITS);
+             MAX_CPUS);
     }
 
     if (endvalue < value) {
@@ -2954,7 +2954,7 @@ int main(int argc, char **argv, char **envp)
 
     for (i = 0; i < MAX_NODES; i++) {
         node_mem[i] = 0;
-        node_cpumask[i] = bitmap_new(MAX_CPUMASK_BITS);
+        node_cpumask[i] = bitmap_new(MAX_CPUS);
     }
 
     nb_numa_nodes = 0;
@@ -4245,7 +4245,7 @@ int main(int argc, char **argv, char **envp)
         }
 
         for (i = 0; i < nb_numa_nodes; i++) {
-            if (!bitmap_empty(node_cpumask[i], MAX_CPUMASK_BITS)) {
+            if (!bitmap_empty(node_cpumask[i], MAX_CPUS)) {
                 break;
             }
         }
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 7/7] vl.c: Use MAX_CPUS macro instead of hardcoded constant

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 64b38a5..62cc734 100644
--- a/vl.c
+++ b/vl.c
@@ -1413,7 +1413,7 @@ static void smp_parse(QemuOpts *opts)
         max_cpus = smp_cpus;
     }
 
-    if (max_cpus > 255) {
+    if (max_cpus > MAX_CPUS) {
         fprintf(stderr, "Unsupported number of maxcpus\n");
         exit(1);
     }
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v4 7/7] vl.c: Use MAX_CPUS macro instead of hardcoded constant

[Laszlo Ersek]

On 03/14/14 20:33, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 64b38a5..62cc734 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1413,7 +1413,7 @@ static void smp_parse(QemuOpts *opts)
>          max_cpus = smp_cpus;
>      }
>  
> -    if (max_cpus > 255) {
> +    if (max_cpus > MAX_CPUS) {
>          fprintf(stderr, "Unsupported number of maxcpus\n");
>          exit(1);
>      }
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v4 0/7] pc: Ensure APIC ID limits before aborting or corrupting memory

[Michael S. Tsirkin]

On Fri, Mar 14, 2014 at 04:33:49PM -0300, Eduardo Habkost wrote:
> Changes v3 -> v4:
>  * Commit message update on patch 5/7
>  * Small comment change (s/should/shall/) on patch 6/7
> 
> Changes v2 -> v3:
>  * Don't use MAX_CPUMASK_BITS on acpi-build.c, use ACPI_CPU_HOTPLUG_ID_LIMIT;
>  * Rename MAX_CPUMASK_BITS to MAX_CPUS, and document it;
>  * Use MAX_CPUS when checking max_cpus limit on vl.c.
> 
> Changes v1 -> v2:
>  * None. v1 was tagged locally by mistake and never submitted to qemu-devel.
> 
> This series adds checks for APIC ID limits on initialization and CPU hotplug
> code.  This fixes multiple issues:
> 
> 1) Assertion failure when -smp parameter results in a too large APIC ID. e.g.:
> 
>     $ ./install/bin/qemu-system-x86_64 -S -smp 254,cores=17,threads=17,sockets=17,maxcpus=254 -nographic
>     **
>     ERROR:hw/acpi/cpu_hotplug.c:58:AcpiCpuHotplug_init: assertion failed: ((id / 8) < ACPI_GPE_PROC_LEN)
>     Aborted (core dumped)
> 
> 2) Memory corruption on AcpiCpuHotplug_add() when APIC ID is too large (similar

Thanks, applied!

> 
> Eduardo Habkost (7):
>   acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro
>   pc: Refuse CPU hotplug if the resulting APIC ID is too large
>   acpi: Assert sts array limit on AcpiCpuHotplug_add()
>   acpi: Don't use MAX_CPUMASK_BITS for APIC ID bitmap
>   pc: Refuse max_cpus if it results in too large APIC ID
>   vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS
>   vl.c: Use MAX_CPUS macro instead of hardcoded constant
> 
>  hw/acpi/cpu_hotplug.c              |  1 +
>  hw/i386/acpi-build.c               |  4 ++--
>  hw/i386/pc.c                       | 16 ++++++++++++++++
>  include/hw/acpi/cpu_hotplug_defs.h |  8 ++++++++
>  include/sysemu/sysemu.h            |  9 ++++++++-
>  vl.c                               | 12 ++++++------
>  6 files changed, 41 insertions(+), 9 deletions(-)
> 
> -- 
> 1.8.5.3

Re: [Qemu-devel] [PATCH v4 6/7] vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS

[Michael S. Tsirkin]

On Fri, Mar 14, 2014 at 04:33:55PM -0300, Eduardo Habkost wrote:
> Also, document what the macro is really useful for.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>


This breaks full build:

CC    hw/ide/macio.o
In file included from hw/ide/macio.c:26:0:
./hw/ppc/mac.h:34:0: error: "MAX_CPUS" redefined [-Werror]
 #define MAX_CPUS 1
 ^
In file included from ./hw/ide/internal.h:12:0,
                 from ./hw/ppc/mac.h:30,
                 from hw/ide/macio.c:26:
/scm/qemu/include/sysemu/sysemu.h:142:0: note: this is the location of
the previous definition
 #define MAX_CPUS 255
 ^
cc1: all warnings being treated as errors
make: *** [hw/ide/macio.o] Error 1

> ---
> Changes v3 -> v4:
>   * s/should/shall/ at the MAX_CPUS comment
> ---
>  include/sysemu/sysemu.h |  9 ++++++++-
>  vl.c                    | 10 +++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b90df9a..6bc1a85 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -133,7 +133,14 @@ extern uint8_t qemu_extra_params_fw[2];
>  extern QEMUClockType rtc_clock;
>  
>  #define MAX_NODES 64
> -#define MAX_CPUMASK_BITS 255
> +
> +/* The following shall be true for all CPUs:
> + *   cpu->cpu_index < max_cpus <= MAX_CPUS
> + *
> + * Note that cpu->get_arch_id() may be larger than MAX_CPUS.
> + */
> +#define MAX_CPUS 255
> +
>  extern int nb_numa_nodes;
>  extern uint64_t node_mem[MAX_NODES];
>  extern unsigned long *node_cpumask[MAX_NODES];
> diff --git a/vl.c b/vl.c
> index bca5c95..64b38a5 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1278,11 +1278,11 @@ static void numa_node_parse_cpus(int nodenr, const char *cpus)
>          goto error;
>      }
>  
> -    if (endvalue >= MAX_CPUMASK_BITS) {
> -        endvalue = MAX_CPUMASK_BITS - 1;
> +    if (endvalue >= MAX_CPUS) {
> +        endvalue = MAX_CPUS - 1;
>          fprintf(stderr,
>              "qemu: NUMA: A max of %d VCPUs are supported\n",
> -             MAX_CPUMASK_BITS);
> +             MAX_CPUS);
>      }
>  
>      if (endvalue < value) {
> @@ -2954,7 +2954,7 @@ int main(int argc, char **argv, char **envp)
>  
>      for (i = 0; i < MAX_NODES; i++) {
>          node_mem[i] = 0;
> -        node_cpumask[i] = bitmap_new(MAX_CPUMASK_BITS);
> +        node_cpumask[i] = bitmap_new(MAX_CPUS);
>      }
>  
>      nb_numa_nodes = 0;
> @@ -4245,7 +4245,7 @@ int main(int argc, char **argv, char **envp)
>          }
>  
>          for (i = 0; i < nb_numa_nodes; i++) {
> -            if (!bitmap_empty(node_cpumask[i], MAX_CPUMASK_BITS)) {
> +            if (!bitmap_empty(node_cpumask[i], MAX_CPUS)) {
>                  break;
>              }
>          }
> -- 
> 1.8.5.3

Re: [Qemu-devel] [PATCH v4 6/7] vl.c: Rename MAX_CPUMASK_BITS to MAX_CPUS

[Eduardo Habkost]

On Fri, Mar 14, 2014 at 04:33:55PM -0300, Eduardo Habkost wrote:
> Also, document what the macro is really useful for.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Michal noted that this broke build. I hadn't noticed that MAX_CPUS was
already defined on some hardware-specific files. Sorry.

As patches 6/7 and 7/7 are not bugfixes, they can wait until 2.1.

-- 
Eduardo