[libvirt] [PATCH 2/2] qemu: domain: Prevent overflows in memory alignment code

[Guido Günther]

Hi,
On Tue, Dec 01, 2015 at 03:11:05PM +0100, Peter Krempa wrote:
> Since libvirt for dubious historical reasons stores memory size as
> kibibytes, it's possible that the alignments done in the qemu code
> overflow the the maximum representable size in bytes. The XML parser
> code handles them in bytes in some stages. Prevent this by doing
> overflow checks when alinging the size and add a test case.

It seems this broke the build on i386:

    https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=i386&ver=1.3.0-1&stamp=1450436203
    (search for memory-align-fail)

I did not investigate further yet though.

Cheers,
 -- Guido

> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260576
> ---
>  src/qemu/qemu_domain.c                             | 27 ++++++++++++++++++++
>  .../qemuxml2argv-memory-align-fail.xml             | 29 ++++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  1 +
>  3 files changed, 57 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-align-fail.xml
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ed21245..a872598 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3521,6 +3521,8 @@ qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def,
>  int
>  qemuDomainAlignMemorySizes(virDomainDefPtr def)
>  {
> +    unsigned long long maxmemkb = virMemoryMaxValue(false) >> 10;
> +    unsigned long long maxmemcapped = virMemoryMaxValue(true) >> 10;
>      unsigned long long initialmem = 0;
>      unsigned long long mem;
>      unsigned long long align = qemuDomainGetMemorySizeAlignment(def);
> @@ -3531,6 +3533,13 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def)
>      for (i = 0; i < ncells; i++) {
>          mem = VIR_ROUND_UP(virDomainNumaGetNodeMemorySize(def->numa, i), align);
>          initialmem += mem;
> +
> +        if (mem > maxmemkb) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("memory size of NUMA node '%zu' overflowed after "
> +                             "alignment"), i);
> +            return -1;
> +        }
>          virDomainNumaSetNodeMemorySize(def->numa, i, mem);
>      }
> 
> @@ -3539,14 +3548,32 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def)
>      if (initialmem == 0)
>          initialmem = VIR_ROUND_UP(virDomainDefGetMemoryInitial(def), align);
> 
> +    if (initialmem > maxmemcapped) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("initial memory size overflowed after alignment"));
> +        return -1;
> +    }
> +
>      virDomainDefSetMemoryInitial(def, initialmem);
> 
>      def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align);
> +    if (def->mem.max_memory > maxmemkb) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("maximum memory size overflowed after alignment"));
> +        return -1;
> +    }
> 
>      /* Align memory module sizes */
>      for (i = 0; i < def->nmems; i++) {
>          align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]);
>          def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align);
> +
> +        if (def->mems[i]->size > maxmemkb) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("size of memory module '%zu' overflowed after "
> +                             "alignment"), i);
> +            return -1;
> +        }
>      }
> 
>      return 0;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-align-fail.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-align-fail.xml
> new file mode 100644
> index 0000000..bdbdc5b
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-align-fail.xml
> @@ -0,0 +1,29 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <maxMemory slots='16' unit='KiB'>9007199254740991</maxMemory>
> +  <memory unit='KiB'>9007199254740991</memory>
> +  <currentMemory unit='KiB'>9007199254740991</currentMemory>
> +  <vcpu placement='static' cpuset='0-1'>2</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <cpu>
> +    <topology sockets='2' cores='1' threads='1'/>
> +    <numa>
> +      <cell id='0' cpus='0-1' memory='9007199254740991' unit='KiB'/>
> +    </numa>
> +  </cpu>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <controller type='ide' index='0'/>
> +    <controller type='usb' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 6513651..37f806e 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1660,6 +1660,7 @@ mymain(void)
>      DO_TEST_PARSE_ERROR("shmem-msi-only", NONE);
>      DO_TEST("cpu-host-passthrough-features", QEMU_CAPS_KVM, QEMU_CAPS_CPU_HOST);
> 
> +    DO_TEST_FAILURE("memory-align-fail", NONE);
>      DO_TEST_FAILURE("memory-hotplug-nonuma", QEMU_CAPS_DEVICE_PC_DIMM);
>      DO_TEST_FAILURE("memory-hotplug", NONE);
>      DO_TEST("memory-hotplug", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA);
> -- 
> 2.6.2
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>