> 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" <stefa...@gmail.com>
> Sent Time: 2018-04-30 22:23:40 (Monday)
> To: "Su Hang" <suhan...@mails.ucas.ac.cn>
> Cc: j...@groklearning.com, j...@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".
