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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature