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

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

[Eduardo Habkost]

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

3) Out of bounds access on node_cpumask on pc_guest_info_init(), if APIC IDs
   are too large.

Eduardo Habkost (4):
  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()
  pc: Refuse max_cpus if it results in too large APIC ID

 hw/acpi/cpu_hotplug.c              |  1 +
 hw/i386/pc.c                       | 17 +++++++++++++++++
 include/hw/acpi/cpu_hotplug_defs.h |  8 ++++++++
 3 files changed, 26 insertions(+)

-- 
1.8.5.3

[Qemu-devel] [PATCH v2 1/4] 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>
---
 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

Re: [Qemu-devel] [PATCH v2 1/4] acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro

[Laszlo Ersek]

On 03/12/14 19:28, Eduardo Habkost wrote:
> 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>
> ---
>  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
>  
> 

You could actually derive ACPI_GPE_PROC_LEN from
ACPI_CPU_HOTPLUG_ID_LIMIT, by dividing with CHAR_BIT. Not too important
because the two macros are adjacent and the comment explains.

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

Re: [Qemu-devel] [PATCH v2 1/4] acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro

[Laszlo Ersek]

On 03/12/14 22:17, Laszlo Ersek wrote:
> On 03/12/14 19:28, Eduardo Habkost wrote:
>> 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>
>> ---
>>  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
>>  
>>
> 
> You could actually derive ACPI_GPE_PROC_LEN from
> ACPI_CPU_HOTPLUG_ID_LIMIT, by dividing with CHAR_BIT.


(Or vice versa -- get the limit from PROC_LEN by multiplication.)
L.

Re: [Qemu-devel] [PATCH v2 1/4] acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro

[Eduardo Habkost]

On Wed, Mar 12, 2014 at 10:18:56PM +0100, Laszlo Ersek wrote:
> On 03/12/14 22:17, Laszlo Ersek wrote:
> > On 03/12/14 19:28, Eduardo Habkost wrote:
> >> 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>
> >> ---
> >>  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
> >>  
> >>
> > 
> > You could actually derive ACPI_GPE_PROC_LEN from
> > ACPI_CPU_HOTPLUG_ID_LIMIT, by dividing with CHAR_BIT.
> 
> 
> (Or vice versa -- get the limit from PROC_LEN by multiplication.)

I tried it, but the ASL compiler doesn't seem to be able to evaluate the
expression.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/4] acpi: Add ACPI_CPU_HOTPLUG_ID_LIMIT macro

[Laszlo Ersek]

On 03/13/14 01:12, Eduardo Habkost wrote:
> On Wed, Mar 12, 2014 at 10:18:56PM +0100, Laszlo Ersek wrote:
>> On 03/12/14 22:17, Laszlo Ersek wrote:
>>> On 03/12/14 19:28, Eduardo Habkost wrote:
>>>> 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>
>>>> ---
>>>>  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
>>>>  
>>>>
>>>
>>> You could actually derive ACPI_GPE_PROC_LEN from
>>> ACPI_CPU_HOTPLUG_ID_LIMIT, by dividing with CHAR_BIT.
>>
>>
>> (Or vice versa -- get the limit from PROC_LEN by multiplication.)
> 
> I tried it, but the ASL compiler doesn't seem to be able to evaluate the
> expression.

Ah right, I did see Igor's comment in the file! OK then.

Thanks,
Laszlo

[Qemu-devel] [PATCH v2 2/4] 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>
---
 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

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

[Laszlo Ersek]

On 03/12/14 19:28, Eduardo Habkost wrote:
> 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>
> ---
>  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);
> 

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

[Qemu-devel] [PATCH v2 3/4] 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>
---
 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

Re: [Qemu-devel] [PATCH v2 3/4] acpi: Assert sts array limit on AcpiCpuHotplug_add()

[Laszlo Ersek]

On 03/12/14 19:28, Eduardo Habkost wrote:
> 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>
> ---
>  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));
>  }
>  
> 

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

[Qemu-devel] [PATCH v2 4/4] 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.

Currently there are two limits we need to check: the CPU hotplug APIC ID
limit (due to the AcpiCpuHotplug.sts array length), and the
MAX_CPUMASK_BITS limit (that's used for CPU bitmaps on NUMA code and
hw/i386/acpi-build.c).

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 74cb4f9..50376a3 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,14 @@ 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
+        || apic_id_limit > MAX_CPUMASK_BITS) {
+        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

Re: [Qemu-devel] [PATCH v2 4/4] pc: Refuse max_cpus if it results in too large APIC ID

[Laszlo Ersek]

comments below

On 03/12/14 19:28, Eduardo Habkost wrote:
> 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.
> 
> Currently there are two limits we need to check: the CPU hotplug APIC ID
> limit (due to the AcpiCpuHotplug.sts array length), and the
> MAX_CPUMASK_BITS limit (that's used for CPU bitmaps on NUMA code and
> hw/i386/acpi-build.c).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/pc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 74cb4f9..50376a3 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;

Seems unnecessary, pc_apic_id_limit() returns "unsigned int".

>  
>      /* init CPUs */
>      if (cpu_model == NULL) {
> @@ -1003,6 +1004,14 @@ 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);

OK, so keep in mind this is an exclusive limit...

> +    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT

... hence this comparison is correct, because ACPI_CPU_HOTPLUG_ID_LIMIT
is also an exclusive limit.

> +        || apic_id_limit > MAX_CPUMASK_BITS) {

However, this check is off-by-one, because MAX_CPUMASK_BITS is an
inclusive limit. It should say (apic_id_limit > MAX_CPUMASK_BITS + 1).

(1) See the type definition AcpiCpuInfo in "hw/i386/acpi-build.c":

typedef struct AcpiCpuInfo {
    DECLARE_BITMAP(found_cpus, MAX_CPUMASK_BITS + 1);
} AcpiCpuInfo;

(2) The acpi_add_cpu_info() function in the same file:

        assert(apic_id <= MAX_CPUMASK_BITS);

On the other hand:

(3) numa_node_parse_cpus():

    if (endvalue >= MAX_CPUMASK_BITS) {
        endvalue = MAX_CPUMASK_BITS - 1;
        fprintf(stderr,
            "qemu: NUMA: A max of %d VCPUs are supported\n",
             MAX_CPUMASK_BITS);
    }

implies an exclusive limit, and:

(4) the two uses in main() also imply an exclusive limit. (Although I
honestly can't find the definition of the bitmap_new() function!)

Since you authored (3) -- in commit c881e20e --, I *think*
MAX_CPUMASK_BITS could indeed be exclusive: then your new patch is
correct, but (1) and (2) -- which seem to be ports from SeaBIOS -- are
"wrong" (as in, "too permissive").

So which is it?

... Aaaah, I understand now! (1) and (2) should actually use
ACPI_CPU_HOTPLUG_ID_LIMIT (which you are just introducing). Basically,
your patchset is completely unrelated to NUMA, the impression arises
*only* because "acpi-build.c" creatively repurposed MAX_CPUMASK_BITS. So
here goes:

- AcpiCpuInfo is already correct *numerically*:

  MAX_CPUMASK_BITS     == 255
  MAX_CPUMASK_BITS + 1 == 256 == ACPI_CPU_HOTPLUG_ID_LIMIT

but it has nothing to do with NUMA -- it should actually use
ACPI_CPU_HOTPLUG_ID_LIMIT. Please include another patch to convert this use.

- acpi_add_cpu_info() is already correct *numerically*:

  assert(apic_id <= MAX_CPUMASK_BITS);
  assert(apic_id <  MAX_CPUMASK_BITS + 1);
  assert(apic_id <  ACPI_CPU_HOTPLUG_ID_LIMIT);

but it has nothing to do with NUMA. Please convert this comparison in
the same additional patch as the AcpiCpuInfo typedef.

- numa_node_parse_cpus() is correct (MAX_CPUMASK_BITS is exclusive). No
need to care about it in this patchset though -- it's NUMA.

- The two "node_cpumask" uses in main() are correct (MAX_CPUMASK_BITS is
exclusive). No need to care about them either in this patchset.

As a result, *this* patch of yours won't mention MAX_CPUMASK_BITS at
all. A single

  (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT)

check will suffice, because ACPI code will use ACPI_CPU_HOTPLUG_ID_LIMIT
exclusively and uniformly, and NUMA code will use the completely
independent MAX_CPUMASK_BITS. (numa_node_parse_cpus(), which is the
function that sets bit segments in the node masks, doesn't care about
APIC IDs at all.)

> +        error_report("max_cpus is too large. APIC ID of last CPU is %lu",
> +                     apic_id_limit - 1);
> +        exit(1);
> +    }
> +

If you update the type of "apic_id_limit" to "unsigned int" (I'm not
saying that you should), don't forget to update the conversion specifier
here.

>      for (i = 0; i < smp_cpus; i++) {
>          cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
>                           icc_bridge, &error);
> 

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2 4/4] pc: Refuse max_cpus if it results in too large APIC ID

[Eduardo Habkost]

On Wed, Mar 12, 2014 at 11:07:38PM +0100, Laszlo Ersek wrote:
> comments below
> 
> On 03/12/14 19:28, Eduardo Habkost wrote:
> > 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.
> > 
> > Currently there are two limits we need to check: the CPU hotplug APIC ID
> > limit (due to the AcpiCpuHotplug.sts array length), and the
> > MAX_CPUMASK_BITS limit (that's used for CPU bitmaps on NUMA code and
> > hw/i386/acpi-build.c).
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/i386/pc.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 74cb4f9..50376a3 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;
> 
> Seems unnecessary, pc_apic_id_limit() returns "unsigned int".
> 
> >  
> >      /* init CPUs */
> >      if (cpu_model == NULL) {
> > @@ -1003,6 +1004,14 @@ 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);
> 
> OK, so keep in mind this is an exclusive limit...
> 
> > +    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT
> 
> ... hence this comparison is correct, because ACPI_CPU_HOTPLUG_ID_LIMIT
> is also an exclusive limit.
> 
> > +        || apic_id_limit > MAX_CPUMASK_BITS) {
> 
> However, this check is off-by-one, because MAX_CPUMASK_BITS is an
> inclusive limit. It should say (apic_id_limit > MAX_CPUMASK_BITS + 1).
> 
> (1) See the type definition AcpiCpuInfo in "hw/i386/acpi-build.c":
> 
> typedef struct AcpiCpuInfo {
>     DECLARE_BITMAP(found_cpus, MAX_CPUMASK_BITS + 1);
> } AcpiCpuInfo;
> 
> (2) The acpi_add_cpu_info() function in the same file:
> 
>         assert(apic_id <= MAX_CPUMASK_BITS);
> 
> On the other hand:
> 
> (3) numa_node_parse_cpus():
> 
>     if (endvalue >= MAX_CPUMASK_BITS) {
>         endvalue = MAX_CPUMASK_BITS - 1;
>         fprintf(stderr,
>             "qemu: NUMA: A max of %d VCPUs are supported\n",
>              MAX_CPUMASK_BITS);
>     }
> 
> implies an exclusive limit, and:
> 
> (4) the two uses in main() also imply an exclusive limit. (Although I
> honestly can't find the definition of the bitmap_new() function!)
> 
> Since you authored (3) -- in commit c881e20e --, I *think*
> MAX_CPUMASK_BITS could indeed be exclusive: then your new patch is
> correct, but (1) and (2) -- which seem to be ports from SeaBIOS -- are
> "wrong" (as in, "too permissive").
> 
> So which is it?
> 
> ... Aaaah, I understand now! (1) and (2) should actually use
> ACPI_CPU_HOTPLUG_ID_LIMIT (which you are just introducing). Basically,
> your patchset is completely unrelated to NUMA, the impression arises
> *only* because "acpi-build.c" creatively repurposed MAX_CPUMASK_BITS. So
> here goes:
> 
> - AcpiCpuInfo is already correct *numerically*:
> 
>   MAX_CPUMASK_BITS     == 255
>   MAX_CPUMASK_BITS + 1 == 256 == ACPI_CPU_HOTPLUG_ID_LIMIT
> 
> but it has nothing to do with NUMA -- it should actually use
> ACPI_CPU_HOTPLUG_ID_LIMIT. Please include another patch to convert this use.

I'll do. Thanks for the thorough analysis!

> 
> - acpi_add_cpu_info() is already correct *numerically*:
> 
>   assert(apic_id <= MAX_CPUMASK_BITS);
>   assert(apic_id <  MAX_CPUMASK_BITS + 1);
>   assert(apic_id <  ACPI_CPU_HOTPLUG_ID_LIMIT);
> 
> but it has nothing to do with NUMA. Please convert this comparison in
> the same additional patch as the AcpiCpuInfo typedef.
> 
> - numa_node_parse_cpus() is correct (MAX_CPUMASK_BITS is exclusive). No
> need to care about it in this patchset though -- it's NUMA.

I was not trying to address ACPI-specific or NUMA-specific stuff, but
all the potential crashes/corruption bugs I found because the code had
the implicit (and incorrect) assumption that apic_id <= max_cpus for all
CPUs.

> 
> - The two "node_cpumask" uses in main() are correct (MAX_CPUMASK_BITS is
> exclusive). No need to care about them either in this patchset.
> 

The MAX_CPUMASK_BITS bits check was added not because of any NUMA code
in vl.c, but because of pc_guest_info_init(), which reads node_cpumask.

...but, wait! The index for node_cpumask is the CPU index, not the APIC
ID! You are right and this patch shouldn't do anything about
MAX_CPUMASK_BITS. I was being overzealous.


> As a result, *this* patch of yours won't mention MAX_CPUMASK_BITS at
> all. A single
> 
>   (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT)
> 
> check will suffice, because ACPI code will use ACPI_CPU_HOTPLUG_ID_LIMIT
> exclusively and uniformly, and NUMA code will use the completely
> independent MAX_CPUMASK_BITS. (numa_node_parse_cpus(), which is the
> function that sets bit segments in the node masks, doesn't care about
> APIC IDs at all.)

You are correct.

I will submit a new version including your suggestions. Thanks!

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Mar 12, 2014 at 03:28:06PM -0300, Eduardo Habkost wrote:
> This series adds checks for APIC ID limits on initialization and CPU hotplug code.
> This fixes multiple issues:
[...]

Please ignore the v2 tag on Subject. This is v1. I was testing with
git-publish and I have made a v1 tag by mistake.

-- 
Eduardo