[Qemu-devel] [PATCH 06/11] acpi_table_add(): extract and reimplement internals

[Laszlo Ersek]

The new function acpi_table_install() installs any blob the caller passes
in. In the next patches this function will be promoted from helper role to
extern.

Reimplementing the logic should make it easier to understand. It also
removes a buffer overflow when

    has_header &&
    cumulative_file_size < ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE

(In that case the g_realloc() call in the read() loop used to shrink the
"acpi_tables" array, causing an out-of-bounds read access when copying the
header out of "acpi_tables".)

The new code isn't more daring alignment-wise than its predecessor:
"acpi_table_header" is packed, and the uint32_t fields are at offsets 6,
26, and 34.

Signed-off-by: Laszlo Ersek <address@hidden>
---
 hw/acpi.c |  284 +++++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 163 insertions(+), 121 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index b29dada..ce408da 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -30,7 +30,7 @@
 
 struct acpi_table_header {
     uint16_t _length;         /* our length, not actual part of the hdr */
-                              /* XXX why we have 2 length fields here? */
+                              /* allows easier parsing for fw_cfg clients */
     char sig[4];              /* ACPI signature (4 ASCII characters) */
     uint32_t length;          /* Length of table, in bytes, including header */
     uint8_t revision;         /* ACPI Specification minor version # */
@@ -45,8 +45,7 @@ struct acpi_table_header {
 #define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
 #define ACPI_TABLE_PFX_SIZE sizeof(uint16_t)  /* size of the extra prefix */
 
-static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE] =
-    "\0\0"                   /* fake _length (2) */
+static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE] =
     "QEMU\0\0\0\0\1\0"       /* sig (4), len(4), revno (1), csum (1) */
     "QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
     "QEMU\1\0\0\0"           /* ASL compiler ID (4), version (4) */
@@ -79,19 +78,165 @@ static int acpi_checksum(const uint8_t *data, int len)
     return (-sum) & 0xff;
 }
 
+
+/* Install a copy of the ACPI table specified in @blob.
+ *
+ * If @has_header is set, @blob starts with the System Description Table Header
+ * structure. Otherwise, "dfl_hdr" is prepended. In any case, each header field
+ * is optionally overwritten from @hdrs.
+ *
+ * It is valid to call this function with
+ * (@blob == NULL && bloblen == 0 && !has_header).
+ *
+ * @hdrs->file and @hdrs->data are ignored.
+ *
+ * SIZE_MAX is considered "infinity" in this function.
+ *
+ * The number of tables that can be installed is not limited, but the 16-bit
+ * counter at the beginning of "acpi_tables" wraps around after UINT16_MAX.
+ */
+static void acpi_table_install(const char unsigned *blob, size_t bloblen,
+                               bool has_header,
+                               const struct AcpiTableOptions *hdrs,
+                               Error **errp)
+{
+    size_t body_start;
+    const char unsigned *hdr_src;
+    size_t body_size, acpi_payload_size;
+    struct acpi_table_header *ext_hdr;
+    unsigned changed_fields;
+
+    /* Calculate where the ACPI table body starts within the blob, plus where
+     * to copy the ACPI table header from.
+     */
+    if (has_header) {
+        /*   _length             | ACPI header in blob | blob body
+         *   ^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^
+         *   ACPI_TABLE_PFX_SIZE     sizeof dfl_hdr      body_size
+         *                           == body_start
+         *
+         *                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+         *                           acpi_payload_size == bloblen
+         */
+        body_start = sizeof dfl_hdr;
+
+        if (bloblen < body_start) {
+            error_setg(errp, "ACPI table claiming to have header is too "
+                       "short, available: %zu, expected: %zu", bloblen,
+                       body_start);
+            return;
+        }
+        hdr_src = blob;
+    } else {
+        /*   _length             | ACPI header in template | blob body
+         *   ^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^
+         *   ACPI_TABLE_PFX_SIZE       sizeof dfl_hdr        body_size
+         *                                                   == bloblen
+         *
+         *                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+         *                                  acpi_payload_size
+         */
+        body_start = 0;
+        hdr_src = dfl_hdr;
+    }
+    body_size = bloblen - body_start;
+    acpi_payload_size = sizeof dfl_hdr + body_size;
+
+    if (acpi_payload_size > UINT16_MAX) {
+        error_setg(errp, "ACPI table too big, requested: %zu, max: %u",
+                   acpi_payload_size, (unsigned)UINT16_MAX);
+        return;
+    }
+
+    /* We won't fail from here on. Initialize / extend the globals. */
+    if (acpi_tables == NULL) {
+        acpi_tables_len = sizeof(uint16_t);
+        acpi_tables = g_malloc0(acpi_tables_len);
+    }
+
+    acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
+                                         ACPI_TABLE_PFX_SIZE +
+                                         sizeof dfl_hdr + body_size);
+
+    ext_hdr = (struct acpi_table_header *)(acpi_tables + acpi_tables_len);
+    acpi_tables_len += ACPI_TABLE_PFX_SIZE;
+
+    memcpy(acpi_tables + acpi_tables_len, hdr_src, sizeof dfl_hdr);
+    acpi_tables_len += sizeof dfl_hdr;
+
+    if (blob != NULL) {
+        memcpy(acpi_tables + acpi_tables_len, blob + body_start, body_size);
+        acpi_tables_len += body_size;
+    }
+
+    /* increase number of tables */
+    cpu_to_le16wu((uint16_t *)acpi_tables,
+                  le16_to_cpupu((uint16_t *)acpi_tables) + 1u);
+
+    /* Update the header fields. The strings need not be NUL-terminated. */
+    changed_fields = 0;
+    ext_hdr->_length = cpu_to_le16(acpi_payload_size);
+
+    if (hdrs->has_sig) {
+        strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
+        ++changed_fields;
+    }
+
+    if (has_header && le32_to_cpu(ext_hdr->length) != acpi_payload_size) {
+        fprintf(stderr,
+                "warning: ACPI table has wrong length, header says "
+                "%" PRIu32 ", actual size %zu bytes\n",
+                le32_to_cpu(ext_hdr->length), acpi_payload_size);
+    }
+    ext_hdr->length = cpu_to_le32(acpi_payload_size);
+
+    if (hdrs->has_rev) {
+        ext_hdr->revision = hdrs->rev;
+        ++changed_fields;
+    }
+
+    ext_hdr->checksum = 0;
+
+    if (hdrs->has_oem_id) {
+        strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
+        ++changed_fields;
+    }
+    if (hdrs->has_oem_table_id) {
+        strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
+                sizeof ext_hdr->oem_table_id);
+        ++changed_fields;
+    }
+    if (hdrs->has_oem_rev) {
+        ext_hdr->oem_revision = cpu_to_le32(hdrs->oem_rev);
+        ++changed_fields;
+    }
+    if (hdrs->has_asl_compiler_id) {
+        strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
+                sizeof ext_hdr->asl_compiler_id);
+        ++changed_fields;
+    }
+    if (hdrs->has_asl_compiler_rev) {
+        ext_hdr->asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
+        ++changed_fields;
+    }
+
+    if (!has_header && changed_fields == 0) {
+        fprintf(stderr, "warning: ACPI table: no headers are specified\n");
+    }
+
+    /* recalculate checksum */
+    ext_hdr->checksum = acpi_checksum((const char unsigned *)ext_hdr +
+                                      ACPI_TABLE_PFX_SIZE, acpi_payload_size);
+}
+
 int acpi_table_add(const QemuOpts *opts)
 {
     AcpiTableOptions *hdrs = NULL;
     Error *err = NULL;
     char **pathnames = NULL;
     char **cur;
-
-    size_t len, start, allen;
-    bool has_header;
-    int changed;
-    int r;
-    struct acpi_table_header hdr;
-    char unsigned *table_start;
+    size_t bloblen = 0;
+    char unsigned *blob = NULL;
 
     {
         OptsVisitor *ov;
@@ -108,7 +253,6 @@ int acpi_table_add(const QemuOpts *opts)
         error_setg(&err, "'-acpitable' requires one of 'data' or 'file'");
         goto out;
     }
-    has_header = hdrs->has_file;
 
     pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0);
     if (pathnames == NULL || pathnames[0] == NULL) {
@@ -116,19 +260,7 @@ int acpi_table_add(const QemuOpts *opts)
         goto out;
     }
 
-    if (!acpi_tables) {
-        allen = sizeof(uint16_t);
-        acpi_tables = g_malloc0(allen);
-    } else {
-        allen = acpi_tables_len;
-    }
-
-    start = allen;
-    acpi_tables = g_realloc(acpi_tables, start + ACPI_TABLE_HDR_SIZE);
-    allen += has_header ? ACPI_TABLE_PFX_SIZE : ACPI_TABLE_HDR_SIZE;
-
     /* now read in the data files, reallocating buffer as needed */
-
     for (cur = pathnames; *cur; ++cur) {
         int fd = open(*cur, O_RDONLY | O_BINARY);
 
@@ -139,13 +271,15 @@ int acpi_table_add(const QemuOpts *opts)
 
         for (;;) {
             char unsigned data[8192];
-            r = read(fd, data, sizeof(data));
+            ssize_t r;
+
+            r = read(fd, data, sizeof data);
             if (r == 0) {
                 break;
             } else if (r > 0) {
-                acpi_tables = g_realloc(acpi_tables, allen + r);
-                memcpy(acpi_tables + allen, data, r);
-                allen += r;
+                blob = g_realloc(blob, bloblen + r);
+                memcpy(blob + bloblen, data, r);
+                bloblen += r;
             } else if (errno != EINTR) {
                 error_setg(&err, "can't read file %s: %s",
                            *cur, strerror(errno));
@@ -157,102 +291,10 @@ int acpi_table_add(const QemuOpts *opts)
         close(fd);
     }
 
-    /* now fill in the header fields */
-
-    table_start = acpi_tables + start;   /* start of the table */
-    changed = 0;
-
-    /* copy the header to temp place to align the fields */
-    memcpy(&hdr, has_header ? table_start : dfl_hdr, ACPI_TABLE_HDR_SIZE);
-
-    /* length of the table minus our prefix */
-    len = allen - start - ACPI_TABLE_PFX_SIZE;
-
-    hdr._length = cpu_to_le16(len);
-
-    if (hdrs->has_sig) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.sig, hdrs->sig, sizeof(hdr.sig));
-        ++changed;
-    }
-
-    /* length of the table including header, in bytes */
-    if (has_header) {
-        unsigned long val;
-
-        /* check if actual length is correct */
-        val = le32_to_cpu(hdr.length);
-        if (val != len) {
-            fprintf(stderr,
-                "warning: acpitable has wrong length,"
-                " header says %lu, actual size %zu bytes\n",
-                val, len);
-            ++changed;
-        }
-    }
-    /* we may avoid putting length here if has_header is true */
-    hdr.length = cpu_to_le32(len);
-
-    if (hdrs->has_rev) {
-        hdr.revision = hdrs->rev;
-        ++changed;
-    }
-
-    if (hdrs->has_oem_id) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.oem_id, hdrs->oem_id, sizeof(hdr.oem_id));
-        ++changed;
-    }
-
-    if (hdrs->has_oem_table_id) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.oem_table_id, hdrs->oem_table_id,
-                sizeof(hdr.oem_table_id));
-        ++changed;
-    }
-
-    if (hdrs->has_oem_rev) {
-        hdr.oem_revision = cpu_to_le32(hdrs->oem_rev);
-        ++changed;
-    }
-
-    if (hdrs->has_asl_compiler_id) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.asl_compiler_id, hdrs->asl_compiler_id,
-                sizeof(hdr.asl_compiler_id));
-        ++changed;
-    }
-
-    if (hdrs->has_asl_compiler_rev) {
-        hdr.asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
-        ++changed;
-    }
-
-    if (!has_header && !changed) {
-        fprintf(stderr, "warning: acpitable: no table headers are 
specified\n");
-    }
-
-
-    /* now calculate checksum of the table, complete with the header */
-    /* we may as well leave checksum intact if has_header is true */
-    /* alternatively there may be a way to set cksum to a given value */
-    hdr.checksum = 0;    /* for checksum calculation */
-
-    /* put header back */
-    memcpy(table_start, &hdr, sizeof(hdr));
-
-    if (changed || !has_header || 1) {
-        ((struct acpi_table_header *)table_start)->checksum =
-            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);
-    }
-
-    /* increase number of tables */
-    (*(uint16_t *)acpi_tables) =
-        cpu_to_le32(le32_to_cpu(*(uint16_t *)acpi_tables) + 1);
-
-    acpi_tables_len = allen;
+    acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, &err);
 
 out:
+    g_free(blob);
     g_strfreev(pathnames);
 
     if (hdrs != NULL) {
-- 
1.7.1