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" <address@hidden>
> Sent Time: 2018-04-30 22:23:40 (Monday)
> To: "Su Hang" <address@hidden>
> Cc: address@hidden, address@hidden, address@hidden
> 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".