[Qemu-devel] [PATCH v6 0/2] Implement Hex file loader and add test case

[Qemu-devel] [PATCH v6 0/2] Implement Hex file loader and add test case

[Su Hang]

These series of patchs implement Intel Hexadecimal File loader and
add QTest testcase to verify the correctness of Loader.

v1: Basic version.
v2: Replace `do{}while(cond);` block with `for(;;)` block.
v3: Add two new files information in MAINTAINERS.
v4: Correct the 'test.hex' path in hexloader-test.c.
v5: Split huge parse_hex_blob() function into four small function;
    Add check for memory bounds;
    Check validation for Record type;
    Replace function ctoh() with glib function g_ascii_xdigit_value();
    Remove check for '.hex' suffix;
    Remove unnecessary type cast;
    Remove duplicate zero-initialization;
    Correct typos;
v6: Call Intel HEX loader after load_uimage_as();
    Remove use of KERNEL_LOAD_ADDR;
    Change (hwaddr) type argument to (hwaddr *) type;
    Use new struct HexParser to contain all arguments needed by
        handle_record_type();
    Enable discontiguous data records (again);
    Remove unnecessary memory clean: bin_buf and HexLine line;
    Correct typo;
    Remove unnecessary check for hex file size;
    Add entry point handle for START_SEG_ADDR_RECORD and
        START_LINEAR_ADDR_RECORD record type;
    Use hwaddr * type to store entry point;

Su Hang (2):
  Implement .hex file loader
  Add QTest testcase for the Intel Hexadecimal Object File Loader.

 MAINTAINERS                          |   6 +
 hw/arm/boot.c                        |  17 ++-
 hw/core/loader.c                     | 241 +++++++++++++++++++++++++++++++++++
 include/hw/loader.h                  |  16 +++
 tests/Makefile.include               |   2 +
 tests/hex-loader-check-data/test.hex |  12 ++
 tests/hexloader-test.c               |  56 ++++++++
 7 files changed, 345 insertions(+), 5 deletions(-)
 create mode 100644 tests/hex-loader-check-data/test.hex
 create mode 100644 tests/hexloader-test.c

--
2.7.4

[Qemu-devel] [PATCH v6 1/2] Implement .hex file loader

[Su Hang]

This patch adds Intel Hexadecimal Object File format support to
the loader.  The file format specification is available here:
http://www.piclist.com/techref/fileext/hex/intel.htm

The file format is mainly intended for embedded systems
and microcontrollers, such as Micro:bit Arduino, ARM, STM32, etc.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 hw/arm/boot.c       |  17 ++--
 hw/core/loader.c    | 241 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h |  16 ++++
 3 files changed, 269 insertions(+), 5 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 26184bcd7c26..15fe5db37762 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1066,16 +1066,23 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
                                      &is_linux, NULL, NULL, as);
     }
+    if (kernel_size < 0) {
+        entry = 0;
+        /* 32-bit ARM Intel HEX file */
+        kernel_size = load_targphys_hex_as(info->kernel_filename, &entry,
+                                           info->ram_size, as);
+    }
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
         kernel_size = load_aarch64_image(info->kernel_filename,
                                          info->loader_start, &entry, as);
         is_linux = 1;
-    } else if (kernel_size < 0) {
-        /* 32-bit ARM */
+    }
+    if (kernel_size < 0) {
+        /* 32-bit ARM binary file */
         entry = info->loader_start + KERNEL_LOAD_ADDR;
-        kernel_size = load_image_targphys_as(info->kernel_filename, entry,
-                                             info->ram_size - KERNEL_LOAD_ADDR,
-                                             as);
+        kernel_size =
+            load_image_targphys_as(info->kernel_filename, entry,
+                                   info->ram_size - KERNEL_LOAD_ADDR, as);
         is_linux = 1;
     }
     if (kernel_size < 0) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 06bdbca53709..cb47276f2cb5 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1286,3 +1286,244 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict)
         }
     }
 }
+
+typedef enum HexRecord HexRecord;
+enum HexRecord {
+    DATA_RECORD = 0,
+    EOF_RECORD,
+    EXT_SEG_ADDR_RECORD,
+    START_SEG_ADDR_RECORD,
+    EXT_LINEAR_ADDR_RECORD,
+    START_LINEAR_ADDR_RECORD,
+};
+
+#define DATA_FIELD_MAX_LEN 0xff
+#define LEN_EXCEPT_DATA 0x5
+/* 0x5 = sizeof(byte_count) + sizeof(address) + sizeof(record_type) +
+ *       sizeof(checksum) */
+typedef struct {
+    uint8_t byte_count;
+    uint16_t address;
+    uint8_t record_type;
+    uint8_t data[DATA_FIELD_MAX_LEN];
+    uint8_t checksum;
+} HexLine;
+
+/* return 0 or -1 if error */
+static size_t parse_record(HexLine *line, uint8_t *our_checksum,
+                           const uint8_t c, const uint32_t idx,
+                           const bool in_process)
+{
+    /* +-------+---------------+-------+---------------------+--------+
+     * | byte  |               |record |                     |        |
+     * | count |    address    | type  |        data         |checksum|
+     * +-------+---------------+-------+---------------------+--------+
+     * ^       ^               ^       ^                     ^        ^
+     * |1 byte |    2 bytes    |1 byte |     0-255 bytes     | 1 byte |
+     */
+    uint8_t value = 0;
+
+    /* ignore space */
+    if (g_ascii_isspace(c)) {
+        return 0;
+    }
+    if (!g_ascii_isxdigit(c) || !in_process) {
+        return -1;
+    }
+    value = g_ascii_xdigit_value(c);
+    value = idx & 0x1 ? value & 0xf : value << 4;
+    if (idx < 2) {
+        line->byte_count |= value;
+    } else if (2 <= idx && idx < 6) {
+        line->address <<= 4;
+        line->address += g_ascii_xdigit_value(c);
+    } else if (6 <= idx && idx < 8) {
+        line->record_type |= value;
+    } else if (8 <= idx && idx < 8 + 2 * line->byte_count) {
+        line->data[(idx - 8) >> 1] |= value;
+    } else if (8 + 2 * line->byte_count <= idx &&
+               idx < 10 + 2 * line->byte_count) {
+        line->checksum |= value;
+    } else {
+        return -1;
+    }
+    *our_checksum += value;
+    return 0;
+}
+
+typedef struct {
+    const char *filename;
+    HexLine line;
+    uint8_t *bin_buf;
+    hwaddr *addr;
+    size_t total_size;
+    uint32_t next_address_to_write;
+    uint32_t current_address;
+    uint32_t current_rom_index;
+    uint32_t rom_start_address;
+    AddressSpace *as;
+} HexParser;
+
+/* return size or -1 if error */
+static size_t handle_record_type(HexParser *parser)
+{
+    HexLine *line = &(parser->line);
+    switch (line->record_type) {
+    case DATA_RECORD:
+        parser->current_address =
+            (parser->next_address_to_write & 0xffff0000) | line->address;
+        /* verify this is a contiguous block of memory */
+        if (parser->current_address != parser->next_address_to_write) {
+            if (parser->current_rom_index != 0) {
+                rom_add_hex_blob_as(parser->filename, parser->bin_buf,
+                                    parser->current_rom_index,
+                                    parser->rom_start_address, parser->as);
+            }
+            parser->rom_start_address = parser->current_address;
+            parser->current_rom_index = 0;
+        }
+
+        /* copy from line buffer to output bin_buf */
+        memcpy(parser->bin_buf + parser->current_rom_index, line->data,
+               line->byte_count);
+        parser->current_rom_index += line->byte_count;
+        parser->total_size += line->byte_count;
+        /* save next address to write */
+        parser->next_address_to_write =
+            parser->current_address + line->byte_count;
+        break;
+
+    case EOF_RECORD:
+        if (parser->current_rom_index != 0) {
+            rom_add_hex_blob_as(parser->filename, parser->bin_buf,
+                                parser->current_rom_index,
+                                parser->rom_start_address, parser->as);
+        }
+        return parser->total_size;
+    case EXT_SEG_ADDR_RECORD:
+    case EXT_LINEAR_ADDR_RECORD:
+        if (line->byte_count != 2 && line->address != 0) {
+            return -1;
+        }
+
+        if (parser->current_rom_index != 0) {
+            rom_add_hex_blob_as(parser->filename, parser->bin_buf,
+                                parser->current_rom_index,
+                                parser->rom_start_address, parser->as);
+        }
+
+        /* save next address to write,
+         * in case of non-contiguous block of memory */
+        parser->next_address_to_write =
+            line->record_type == EXT_SEG_ADDR_RECORD
+                ? ((line->data[0] << 12) | (line->data[1] << 4))
+                : ((line->data[0] << 24) | (line->data[1] << 16));
+        parser->rom_start_address = parser->next_address_to_write;
+        parser->current_rom_index = 0;
+        break;
+
+    case START_SEG_ADDR_RECORD:
+    case START_LINEAR_ADDR_RECORD:
+        *(parser->addr) = (line->data[0] << 24) | (line->data[1] << 16) |
+                          (line->data[2] << 8) | (line->data[3]);
+        break;
+
+    default:
+        return -1;
+    }
+
+    return parser->total_size;
+}
+
+/* return size or -1 if error */
+static size_t parse_hex_blob(const char *filename, hwaddr *addr,
+                             uint8_t *hex_blob, off_t hex_blob_size,
+                             uint8_t *bin_buf, AddressSpace *as)
+{
+    bool in_process = false; /* avoid re-enter and
+                              * check whether record begin with ':' */
+    uint8_t *end = hex_blob + hex_blob_size;
+    uint8_t our_checksum = 0;
+    uint32_t record_index = 0;
+    HexParser parser = {0};
+    parser.filename = filename;
+    parser.bin_buf = bin_buf;
+    parser.addr = addr;
+    parser.as = as;
+
+    for (; hex_blob < end; ++hex_blob) {
+        switch (*hex_blob) {
+        case '\r':
+        case '\n':
+            if (!in_process) {
+                break;
+            }
+
+            in_process = false;
+            if ((record_index >> 1) - LEN_EXCEPT_DATA !=
+                    parser.line.byte_count ||
+                our_checksum != 0) {
+                return -1;
+            }
+
+            if (handle_record_type(&parser) == -1) {
+                return -1;
+            }
+            break;
+
+        /* start of a new record. */
+        case ':':
+            memset(&parser.line, 0, sizeof(HexLine));
+            in_process = true;
+            record_index = 0;
+            break;
+
+        /* decoding lines */
+        default:
+            if (parse_record(&parser.line, &our_checksum, *hex_blob,
+                             record_index, in_process) != 0) {
+                return -1;
+            }
+            ++record_index;
+            break;
+        }
+    }
+    return parser.total_size;
+}
+
+/* return size or -1 if error */
+int load_targphys_hex_as(const char *filename, hwaddr *addr, uint64_t max_sz,
+                         AddressSpace *as)
+{
+    int fd;
+    off_t hex_blob_size;
+    uint8_t *hex_blob;
+    uint8_t *bin_buf;
+    size_t total_size = 0;
+
+    fd = open(filename, O_RDONLY);
+    if (fd < 0) {
+        return -1;
+    }
+    hex_blob_size = lseek(fd, 0, SEEK_END);
+    if (hex_blob_size < 0) {
+        close(fd);
+        return -1;
+    }
+    hex_blob = g_malloc(hex_blob_size);
+    bin_buf = g_malloc(hex_blob_size);
+    lseek(fd, 0, SEEK_SET);
+    if (read(fd, hex_blob, hex_blob_size) != hex_blob_size) {
+        total_size = -1;
+        goto hex_parser_exit;
+    }
+
+    total_size =
+        parse_hex_blob(filename, addr, hex_blob, hex_blob_size, bin_buf, as);
+
+hex_parser_exit:
+    close(fd);
+    g_free(hex_blob);
+    g_free(bin_buf);
+    return total_size;
+}
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 5ed3fd8ae67a..728198a91ef9 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -28,6 +28,20 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
 int load_image_targphys_as(const char *filename,
                            hwaddr addr, uint64_t max_sz, AddressSpace *as);
 
+/**load_image_targphys_hex_as:
+ * @filename: Path to the .hex file
+ * @addr: Store the entry point get from .hex file
+ * @max_sz: The maximum size of the .hex file to load
+ * @as: The AddressSpace to load the .hex file to. The value of
+ *      address_space_memory is used if nothing is supplied here.
+ *
+ * Load a fixed .hex file into memory.
+ *
+ * Returns the size of the loaded .hex file on success, -1 otherwise.
+ */
+int load_targphys_hex_as(const char *filename,
+                         hwaddr *addr, uint64_t max_sz, AddressSpace *as);
+
 /** load_image_targphys:
  * Same as load_image_targphys_as(), but doesn't allow the caller to specify
  * an AddressSpace.
@@ -241,6 +255,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
     rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
 #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
+#define rom_add_hex_blob_as(_f, _b, _l, _a, _as)      \
+    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 1/2] Implement .hex file loader

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 8336 bytes --]

On Thu, Apr 26, 2018 at 09:51:37PM +0800, Su Hang wrote:
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
>          kernel_size = load_aarch64_image(info->kernel_filename,
>                                           info->loader_start, &entry, as);
>          is_linux = 1;
> -    } else if (kernel_size < 0) {
> -        /* 32-bit ARM */
> +    }
> +    if (kernel_size < 0) {
> +        /* 32-bit ARM binary file */
>          entry = info->loader_start + KERNEL_LOAD_ADDR;
> -        kernel_size = load_image_targphys_as(info->kernel_filename, entry,
> -                                             info->ram_size - KERNEL_LOAD_ADDR,
> -                                             as);
> +        kernel_size =
> +            load_image_targphys_as(info->kernel_filename, entry,
> +                                   info->ram_size - KERNEL_LOAD_ADDR, as);

These changes seem unnecessary.

> +/* return 0 or -1 if error */
> +static size_t parse_record(HexLine *line, uint8_t *our_checksum,

size_t is unsigned, so returning -1 is not ideal.  This function only
needs to return success or failure.  Please use bool instead.

> +typedef struct {
> +    const char *filename;
> +    HexLine line;
> +    uint8_t *bin_buf;
> +    hwaddr *addr;
> +    size_t total_size;

Please use int instead of size_t since this is the return value from
load_image_hex_as().

> +    uint32_t next_address_to_write;
> +    uint32_t current_address;
> +    uint32_t current_rom_index;
> +    uint32_t rom_start_address;
> +    AddressSpace *as;
> +} HexParser;
> +
> +/* return size or -1 if error */
> +static size_t handle_record_type(HexParser *parser)

Please use int instead of size_t (see above for reasons).

> +{
> +    HexLine *line = &(parser->line);
> +    switch (line->record_type) {
> +    case DATA_RECORD:
> +        parser->current_address =
> +            (parser->next_address_to_write & 0xffff0000) | line->address;
> +        /* verify this is a contiguous block of memory */
> +        if (parser->current_address != parser->next_address_to_write) {
> +            if (parser->current_rom_index != 0) {
> +                rom_add_hex_blob_as(parser->filename, parser->bin_buf,
> +                                    parser->current_rom_index,
> +                                    parser->rom_start_address, parser->as);
> +            }
> +            parser->rom_start_address = parser->current_address;
> +            parser->current_rom_index = 0;
> +        }
> +
> +        /* copy from line buffer to output bin_buf */
> +        memcpy(parser->bin_buf + parser->current_rom_index, line->data,
> +               line->byte_count);
> +        parser->current_rom_index += line->byte_count;
> +        parser->total_size += line->byte_count;
> +        /* save next address to write */
> +        parser->next_address_to_write =
> +            parser->current_address + line->byte_count;
> +        break;
> +
> +    case EOF_RECORD:
> +        if (parser->current_rom_index != 0) {
> +            rom_add_hex_blob_as(parser->filename, parser->bin_buf,
> +                                parser->current_rom_index,
> +                                parser->rom_start_address, parser->as);
> +        }
> +        return parser->total_size;
> +    case EXT_SEG_ADDR_RECORD:
> +    case EXT_LINEAR_ADDR_RECORD:
> +        if (line->byte_count != 2 && line->address != 0) {
> +            return -1;
> +        }
> +
> +        if (parser->current_rom_index != 0) {
> +            rom_add_hex_blob_as(parser->filename, parser->bin_buf,
> +                                parser->current_rom_index,
> +                                parser->rom_start_address, parser->as);
> +        }
> +
> +        /* save next address to write,
> +         * in case of non-contiguous block of memory */
> +        parser->next_address_to_write =
> +            line->record_type == EXT_SEG_ADDR_RECORD
> +                ? ((line->data[0] << 12) | (line->data[1] << 4))
> +                : ((line->data[0] << 24) | (line->data[1] << 16));
> +        parser->rom_start_address = parser->next_address_to_write;
> +        parser->current_rom_index = 0;
> +        break;
> +
> +    case START_SEG_ADDR_RECORD:

START_SEG_ADDR_RECORD is x86-specific and not implemented by this patch
(the address is given in CS:IP real-mode addressing format and you would
need to calculate the linear address).  Please return an error if this
record type is encountered.

> +    case START_LINEAR_ADDR_RECORD:

Please check that byte_count == 4 and address == 0.

> +        *(parser->addr) = (line->data[0] << 24) | (line->data[1] << 16) |
> +                          (line->data[2] << 8) | (line->data[3]);

Please name the field start_addr so its purpose is clear.

> +        break;
> +
> +    default:
> +        return -1;
> +    }
> +
> +    return parser->total_size;
> +}
> +
> +/* return size or -1 if error */
> +static size_t parse_hex_blob(const char *filename, hwaddr *addr,
> +                             uint8_t *hex_blob, off_t hex_blob_size,
> +                             uint8_t *bin_buf, AddressSpace *as)

Please use int instead of size_t (see above for reasons).

> +{
> +    bool in_process = false; /* avoid re-enter and
> +                              * check whether record begin with ':' */
> +    uint8_t *end = hex_blob + hex_blob_size;
> +    uint8_t our_checksum = 0;
> +    uint32_t record_index = 0;
> +    HexParser parser = {0};
> +    parser.filename = filename;
> +    parser.bin_buf = bin_buf;
> +    parser.addr = addr;
> +    parser.as = as;
> +
> +    for (; hex_blob < end; ++hex_blob) {
> +        switch (*hex_blob) {
> +        case '\r':
> +        case '\n':
> +            if (!in_process) {
> +                break;
> +            }
> +
> +            in_process = false;
> +            if ((record_index >> 1) - LEN_EXCEPT_DATA !=
> +                    parser.line.byte_count ||

A malicious input file can control the following values:
 * record_index using whitespace (see "case default" below)
 * byte_count in range [0x00, 0xff]
 * our_checksum = 0 by choosing the right address field values

The input file can use '\n' before any data bytes are read:

  :<whitespace>ff000100\n

The number of whitespace needs to be calculated so that the byte_count
comparison succeeds:

  if ((520 >> 1) - 5 != 0xff || ...)

Therefore 520 - strlen("ff000100") = 512 whitespace characters are
required to bypass this check.

Here is what happens: this if statement returns true and
handle_record_type() can be used to load uninitialized heap memory from
bin_buf into the guest.

This is a memory disclosure bug.  The guest must not be able to access
data from QEMU's heap for security reasons (e.g. it can be used to make
additional exploits easier by revealing memory addresses).  QEMU cannot
trust the input file!

record_index must only be incremented when a hex record byte has been
processed, not for whitespace.  I also suggest rewriting the expression
to avoid unsigned underflow and odd division by 2 (4 >> 1 == 2 but 5 >>
1 == 2 as well!):

  if (LEN_EXCEPT_DATA + parser.line.byte_count * 2 != record_index ||

> +/* return size or -1 if error */
> +int load_targphys_hex_as(const char *filename, hwaddr *addr, uint64_t max_sz,

max_sz is unused.

> +                         AddressSpace *as)
> +{
> +    int fd;
> +    off_t hex_blob_size;
> +    uint8_t *hex_blob;
> +    uint8_t *bin_buf;
> +    size_t total_size = 0;

Please use int instead of size_t (see above for reasons).

> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 5ed3fd8ae67a..728198a91ef9 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -28,6 +28,20 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
>  int load_image_targphys_as(const char *filename,
>                             hwaddr addr, uint64_t max_sz, AddressSpace *as);
>  
> +/**load_image_targphys_hex_as:
> + * @filename: Path to the .hex file
> + * @addr: Store the entry point get from .hex file

"addr" is vague, please name this argument "entry".

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] Implement .hex file loader

[Su Hang]

> A malicious input file can control the following values:
>  * record_index using whitespace (see "case default" below)
>  * byte_count in range [0x00, 0xff]
>  * our_checksum = 0 by choosing the right address field values

Oh, that is really a disaster...
Thanks for your review.

Su Hang

> -----Original Messages-----
> From: "Stefan Hajnoczi" <stefanha@gmail.com>
> Sent Time: 2018-04-30 22:23:40 (Monday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: jim@groklearning.com, joel@jms.id.au, qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v6 1/2] Implement .hex file loader
> 
> On Thu, Apr 26, 2018 at 09:51:37PM +0800, Su Hang wrote:
> >      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
> >          kernel_size = load_aarch64_image(info->kernel_filename,
> >                                           info->loader_start, &entry, as);
> >          is_linux = 1;
> > -    } else if (kernel_size < 0) {
> > -        /* 32-bit ARM */
> > +    }
> > +    if (kernel_size < 0) {
> > +        /* 32-bit ARM binary file */
> >          entry = info->loader_start + KERNEL_LOAD_ADDR;
> > -        kernel_size = load_image_targphys_as(info->kernel_filename, entry,
> > -                                             info->ram_size - KERNEL_LOAD_ADDR,
> > -                                             as);
> > +        kernel_size =
> > +            load_image_targphys_as(info->kernel_filename, entry,
> > +                                   info->ram_size - KERNEL_LOAD_ADDR, as);
> 
> These changes seem unnecessary.
> 
> > +/* return 0 or -1 if error */
> > +static size_t parse_record(HexLine *line, uint8_t *our_checksum,
> 
> size_t is unsigned, so returning -1 is not ideal.  This function only
> needs to return success or failure.  Please use bool instead.
> 
> > +typedef struct {
> > +    const char *filename;
> > +    HexLine line;
> > +    uint8_t *bin_buf;
> > +    hwaddr *addr;
> > +    size_t total_size;
> 
> Please use int instead of size_t since this is the return value from
> load_image_hex_as().
> 
> > +    uint32_t next_address_to_write;
> > +    uint32_t current_address;
> > +    uint32_t current_rom_index;
> > +    uint32_t rom_start_address;
> > +    AddressSpace *as;
> > +} HexParser;
> > +
> > +/* return size or -1 if error */
> > +static size_t handle_record_type(HexParser *parser)
> 
> Please use int instead of size_t (see above for reasons).
> 
> > +{
> > +    HexLine *line = &(parser->line);
> > +    switch (line->record_type) {
> > +    case DATA_RECORD:
> > +        parser->current_address =
> > +            (parser->next_address_to_write & 0xffff0000) | line->address;
> > +        /* verify this is a contiguous block of memory */
> > +        if (parser->current_address != parser->next_address_to_write) {
> > +            if (parser->current_rom_index != 0) {
> > +                rom_add_hex_blob_as(parser->filename, parser->bin_buf,
> > +                                    parser->current_rom_index,
> > +                                    parser->rom_start_address, parser->as);
> > +            }
> > +            parser->rom_start_address = parser->current_address;
> > +            parser->current_rom_index = 0;
> > +        }
> > +
> > +        /* copy from line buffer to output bin_buf */
> > +        memcpy(parser->bin_buf + parser->current_rom_index, line->data,
> > +               line->byte_count);
> > +        parser->current_rom_index += line->byte_count;
> > +        parser->total_size += line->byte_count;
> > +        /* save next address to write */
> > +        parser->next_address_to_write =
> > +            parser->current_address + line->byte_count;
> > +        break;
> > +
> > +    case EOF_RECORD:
> > +        if (parser->current_rom_index != 0) {
> > +            rom_add_hex_blob_as(parser->filename, parser->bin_buf,
> > +                                parser->current_rom_index,
> > +                                parser->rom_start_address, parser->as);
> > +        }
> > +        return parser->total_size;
> > +    case EXT_SEG_ADDR_RECORD:
> > +    case EXT_LINEAR_ADDR_RECORD:
> > +        if (line->byte_count != 2 && line->address != 0) {
> > +            return -1;
> > +        }
> > +
> > +        if (parser->current_rom_index != 0) {
> > +            rom_add_hex_blob_as(parser->filename, parser->bin_buf,
> > +                                parser->current_rom_index,
> > +                                parser->rom_start_address, parser->as);
> > +        }
> > +
> > +        /* save next address to write,
> > +         * in case of non-contiguous block of memory */
> > +        parser->next_address_to_write =
> > +            line->record_type == EXT_SEG_ADDR_RECORD
> > +                ? ((line->data[0] << 12) | (line->data[1] << 4))
> > +                : ((line->data[0] << 24) | (line->data[1] << 16));
> > +        parser->rom_start_address = parser->next_address_to_write;
> > +        parser->current_rom_index = 0;
> > +        break;
> > +
> > +    case START_SEG_ADDR_RECORD:
> 
> START_SEG_ADDR_RECORD is x86-specific and not implemented by this patch
> (the address is given in CS:IP real-mode addressing format and you would
> need to calculate the linear address).  Please return an error if this
> record type is encountered.
> 
> > +    case START_LINEAR_ADDR_RECORD:
> 
> Please check that byte_count == 4 and address == 0.
> 
> > +        *(parser->addr) = (line->data[0] << 24) | (line->data[1] << 16) |
> > +                          (line->data[2] << 8) | (line->data[3]);
> 
> Please name the field start_addr so its purpose is clear.
> 
> > +        break;
> > +
> > +    default:
> > +        return -1;
> > +    }
> > +
> > +    return parser->total_size;
> > +}
> > +
> > +/* return size or -1 if error */
> > +static size_t parse_hex_blob(const char *filename, hwaddr *addr,
> > +                             uint8_t *hex_blob, off_t hex_blob_size,
> > +                             uint8_t *bin_buf, AddressSpace *as)
> 
> Please use int instead of size_t (see above for reasons).
> 
> > +{
> > +    bool in_process = false; /* avoid re-enter and
> > +                              * check whether record begin with ':' */
> > +    uint8_t *end = hex_blob + hex_blob_size;
> > +    uint8_t our_checksum = 0;
> > +    uint32_t record_index = 0;
> > +    HexParser parser = {0};
> > +    parser.filename = filename;
> > +    parser.bin_buf = bin_buf;
> > +    parser.addr = addr;
> > +    parser.as = as;
> > +
> > +    for (; hex_blob < end; ++hex_blob) {
> > +        switch (*hex_blob) {
> > +        case '\r':
> > +        case '\n':
> > +            if (!in_process) {
> > +                break;
> > +            }
> > +
> > +            in_process = false;
> > +            if ((record_index >> 1) - LEN_EXCEPT_DATA !=
> > +                    parser.line.byte_count ||
> 
> A malicious input file can control the following values:
>  * record_index using whitespace (see "case default" below)
>  * byte_count in range [0x00, 0xff]
>  * our_checksum = 0 by choosing the right address field values
> 
> The input file can use '\n' before any data bytes are read:
> 
>   :<whitespace>ff000100\n
> 
> The number of whitespace needs to be calculated so that the byte_count
> comparison succeeds:
> 
>   if ((520 >> 1) - 5 != 0xff || ...)
> 
> Therefore 520 - strlen("ff000100") = 512 whitespace characters are
> required to bypass this check.
> 
> Here is what happens: this if statement returns true and
> handle_record_type() can be used to load uninitialized heap memory from
> bin_buf into the guest.
> 
> This is a memory disclosure bug.  The guest must not be able to access
> data from QEMU's heap for security reasons (e.g. it can be used to make
> additional exploits easier by revealing memory addresses).  QEMU cannot
> trust the input file!
> 
> record_index must only be incremented when a hex record byte has been
> processed, not for whitespace.  I also suggest rewriting the expression
> to avoid unsigned underflow and odd division by 2 (4 >> 1 == 2 but 5 >>
> 1 == 2 as well!):
> 
>   if (LEN_EXCEPT_DATA + parser.line.byte_count * 2 != record_index ||
> 
> > +/* return size or -1 if error */
> > +int load_targphys_hex_as(const char *filename, hwaddr *addr, uint64_t max_sz,
> 
> max_sz is unused.
> 
> > +                         AddressSpace *as)
> > +{
> > +    int fd;
> > +    off_t hex_blob_size;
> > +    uint8_t *hex_blob;
> > +    uint8_t *bin_buf;
> > +    size_t total_size = 0;
> 
> Please use int instead of size_t (see above for reasons).
> 
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index 5ed3fd8ae67a..728198a91ef9 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -28,6 +28,20 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
> >  int load_image_targphys_as(const char *filename,
> >                             hwaddr addr, uint64_t max_sz, AddressSpace *as);
> >  
> > +/**load_image_targphys_hex_as:
> > + * @filename: Path to the .hex file
> > + * @addr: Store the entry point get from .hex file
> 
> "addr" is vague, please name this argument "entry".

Re: [Qemu-devel] [PATCH v6 1/2] Implement .hex file loader

[Su Hang]

> > +    size_t total_size = 0;
> 
> Please use int instead of size_t (see above for reasons).
I have question here:
Since `EXT_LINEAR_ADDR_RECORD` supports 32 bit addressing (up to 4GiB), is `int`
big enough for this type?
The same question will happen to other similar cases.
Or should I use `long long` instead of `int`?

Su Hang

Re: [Qemu-devel] [PATCH v6 1/2] Implement .hex file loader

[Stefan Hajnoczi]

On Mon, May 7, 2018 at 10:27 AM, Su Hang <suhang16@mails.ucas.ac.cn> wrote:
>> > +    size_t total_size = 0;
>>
>> Please use int instead of size_t (see above for reasons).
> I have question here:
> Since `EXT_LINEAR_ADDR_RECORD` supports 32 bit addressing (up to 4GiB), is `int`
> big enough for this type?
> The same question will happen to other similar cases.
> Or should I use `long long` instead of `int`?

There is no way of fixing that without modifying hw/arm/boot.c to use
a different type for kernel_size.  It looks like the actual size is
never used in that file.  A bool would be enough to indicate whether
loading the kernel succeeded.

Then the Intel .hex file parser code could be adjusted to either just
return bool or to use an appropriate type as you suggested.

Stefan

[Qemu-devel] [PATCH v6 2/2] Add QTest testcase for the Intel Hexadecimal Object File Loader.

[Su Hang]

'test.hex' file is a bare metal ARM software stored in Hexadecimal
Object Format. When it's loaded by QEMU, it will print "Hello world!\n"
on console.

`pre_store` array in 'hexloader-test.c' file, stores the binary format
of 'test.hex' file, which is used to verify correctness.

Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 MAINTAINERS                          |  6 ++++
 tests/Makefile.include               |  2 ++
 tests/hex-loader-check-data/test.hex | 12 ++++++++
 tests/hexloader-test.c               | 56 ++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+)
 create mode 100644 tests/hex-loader-check-data/test.hex
 create mode 100644 tests/hexloader-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 24b70169bc37..3d37d04c3345 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1291,6 +1291,12 @@ F: hw/core/generic-loader.c
 F: include/hw/core/generic-loader.h
 F: docs/generic-loader.txt
 
+Intel Hexadecimal Object File Loader
+M: Su Hang <suhang16@mails.ucas.ac.cn>
+S: Maintained
+F: tests/hexloader-test.c
+F: tests/hex-loader-check-data/test.hex
+
 CHRP NVRAM
 M: Thomas Huth <thuth@redhat.com>
 S: Maintained
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2c2..f4a3e71f34ee 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -380,6 +380,7 @@ check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 gcov-files-arm-y += hw/timer/arm_mptimer.c
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/sdhci-test$(EXESUF)
+check-qtest-arm-y += tests/hexloader-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
@@ -755,6 +756,7 @@ tests/qmp-test$(EXESUF): tests/qmp-test.o
 tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
+tests/hexloader-test$(EXESUF): tests/hexloader-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
diff --git a/tests/hex-loader-check-data/test.hex b/tests/hex-loader-check-data/test.hex
new file mode 100644
index 000000000000..7e99b452f5cc
--- /dev/null
+++ b/tests/hex-loader-check-data/test.hex
@@ -0,0 +1,12 @@
+:020000040001F9
+:1000000004D09FE5160000EBFEFFFFEA9810010008
+:1000100004B02DE500B08DE20CD04DE208000BE5F8
+:10002000060000EA08301BE50020D3E52C309FE5F0
+:10003000002083E508301BE5013083E208300BE542
+:1000400008301BE50030D3E5000053E3F4FFFF1A4E
+:100050000000A0E100D08BE204B09DE41EFF2FE180
+:1000600000101F1000482DE904B08DE208009FE544
+:10007000E6FFFFEB0000A0E10088BDE8840001007E
+:1000800000101F1048656C6C6F20776F726C6421D4
+:020090000A0064
+:00000001FF
diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c
new file mode 100644
index 000000000000..70f99ac03c6b
--- /dev/null
+++ b/tests/hexloader-test.c
@@ -0,0 +1,56 @@
+/*
+ * QTest testcase for the Intel Hexadecimal Object File Loader
+ *
+ * Authors:
+ *  Su Hang <suhang16@mails.ucas.ac.cn> 2018
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+#define BIN_SIZE 146
+
+static unsigned char pre_store[BIN_SIZE] = {
+    4,   208, 159, 229, 22,  0,   0,   235, 254, 255, 255, 234, 152, 16,  1,
+    0,   4,   176, 45,  229, 0,   176, 141, 226, 12,  208, 77,  226, 8,   0,
+    11,  229, 6,   0,   0,   234, 8,   48,  27,  229, 0,   32,  211, 229, 44,
+    48,  159, 229, 0,   32,  131, 229, 8,   48,  27,  229, 1,   48,  131, 226,
+    8,   48,  11,  229, 8,   48,  27,  229, 0,   48,  211, 229, 0,   0,   83,
+    227, 244, 255, 255, 26,  0,   0,   160, 225, 0,   208, 139, 226, 4,   176,
+    157, 228, 30,  255, 47,  225, 0,   16,  31,  16,  0,   72,  45,  233, 4,
+    176, 141, 226, 8,   0,   159, 229, 230, 255, 255, 235, 0,   0,   160, 225,
+    0,   136, 189, 232, 132, 0,   1,   0,   0,   16,  31,  16,  72,  101, 108,
+    108, 111, 32,  119, 111, 114, 108, 100, 33,  10,  0};
+
+/* success if no crash or abort */
+static void hex_loader_test(void)
+{
+    unsigned int i;
+    unsigned char memory_content[BIN_SIZE];
+    const unsigned int base_addr = 0x00010000;
+
+    QTestState *s = qtest_startf(
+        "-M versatilepb -m 128M -nographic -kernel ../tests/hex-loader-check-data/test.hex");
+
+    for (i = 0; i < BIN_SIZE; ++i) {
+        memory_content[i] = qtest_readb(s, base_addr + i);
+        g_assert_cmpuint(memory_content[i], ==, pre_store[i]);
+    }
+    qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/tmp/hex_loader", hex_loader_test);
+    ret = g_test_run();
+
+    return ret;
+}
-- 
2.7.4