On Wed, Mar 06, 2013 at 09:48:11AM -0500, Jeff Cody wrote: > +#define leguid_to_cpus(guid) do { \ > + le32_to_cpus(&(guid)->data1); \ > + le16_to_cpus(&(guid)->data2); \ > + le16_to_cpus(&(guid)->data3); } while (0)
This should be a function. Please avoid macros. > +static const ms_guid logical_sector_guid = {.data1 = 0x8141bf1d, > + .data2 = 0xa96f, > + .data3 = 0x4709, > + .data4 = { 0xba, 0x47, 0xf2, 0x33, Indentation > + 0xa8, 0xfa, 0xab, > 0x5f} }; > + > +static const ms_guid phys_sector_guid = { .data1 = 0xcda348c7, > + .data2 = 0x445d, > + .data3 = 0x4471, > + .data4 = { 0x9c, 0xc9, 0xe9, 0x88, > + 0x52, 0x51, 0xc5, > 0x56} }; > + > +static const ms_guid parent_locator_guid = {.data1 = 0xa8d35f2d, > + .data2 = 0xb30b, > + .data3 = 0x454d, > + .data4 = { 0xab, 0xf7, 0xd3, 0xd8, Indentation > +typedef struct BDRVVHDXState { > + CoMutex lock; > + > + int curr_header; > + vhdx_header *headers[2]; > + > + vhdx_region_table_header rt; > + vhdx_region_table_entry bat_rt; /* region table for the BAT */ > + vhdx_region_table_entry metadata_rt; /* region table for the metadata > */ > + vhdx_region_table_entry *unknown_rt; > + unsigned int unknown_rt_size; This field isn't used yet but "unsigned int" for something that has "size" in its name is suspicious. Should it be size_t (memory) or uint64_t (disk)? > +static int vhdx_probe(const uint8_t *buf, int buf_size, const char *filename) > +{ > + if (buf_size >= 8 && !strncmp((char *)buf, "vhdxfile", 8)) { memcmp() saves you the trouble of casting buf (which should stay const, BTW). > +static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s) > +{ > + int ret = 0; > + uint8_t *buffer; > + int offset = 0; > + int i = 0; > + uint32_t block_size, sectors_per_block, logical_sector_size; > + uint64_t chunk_ratio; > + vhdx_metadata_table_entry md_entry; > + > + buffer = g_malloc(VHDX_METADATA_TABLE_MAX_SIZE); Please use qemu_blockalign() to avoid bounce buffers in case the memory is not 512-byte aligned. Use qemu_vfree() instead of g_free(). This applies to all I/O buffers that the block driver allocates. > + > + ret = bdrv_pread(bs->file, s->metadata_rt.file_offset, buffer, > + VHDX_METADATA_TABLE_MAX_SIZE); > + if (ret < 0) { > + goto fail_no_free; > + } > + memcpy(&s->metadata_hdr, buffer, sizeof(s->metadata_hdr)); > + offset += sizeof(s->metadata_hdr); > + > + le64_to_cpus(&s->metadata_hdr.signature); > + le16_to_cpus(&s->metadata_hdr.reserved); > + le16_to_cpus(&s->metadata_hdr.entry_count); > + > + if (s->metadata_hdr.signature != VHDX_METADATA_MAGIC) { > + ret = -EINVAL; > + goto fail_no_free; > + } > + > + s->metadata_entries.present = 0; > + > + for (i = 0; i < s->metadata_hdr.entry_count; i++) { > + memcpy(&md_entry, buffer+offset, sizeof(md_entry)); Read beyond end of buffer if metadata_hdr.entry_count is wrong. We cannot trust the image file - it can be corrupt or malicious. Checks are required for everything that can be validated. QEMU cannot do anything that would cause a crash or memory corruption on invalid input. > + ret = bdrv_pread(bs->file, > + s->metadata_entries.file_parameters_entry.offset > + + s->metadata_rt.file_offset, > + &s->params, > + sizeof(s->params)); Missing error code path when ret < 0. > + /* determine virtual disk size, logical sector size, > + * and phys sector size */ > + > + ret = bdrv_pread(bs->file, > + s->metadata_entries.virtual_disk_size_entry.offset > + + s->metadata_rt.file_offset, > + &s->virtual_disk_size, > + sizeof(uint64_t)); > + if (ret < 0) { > + goto exit; > + } > + ret = bdrv_pread(bs->file, > + s->metadata_entries.logical_sector_size_entry.offset > + + s->metadata_rt.file_offset, > + &s->logical_sector_size, > + sizeof(uint32_t)); > + if (ret < 0) { > + goto exit; > + } > + ret = bdrv_pread(bs->file, > + s->metadata_entries.phys_sector_size_entry.offset > + + s->metadata_rt.file_offset, > + &s->physical_sector_size, > + sizeof(uint32_t)); > + if (ret < 0) { > + goto exit; > + } > + > + le64_to_cpus(&s->virtual_disk_size); > + le32_to_cpus(&s->logical_sector_size); > + le32_to_cpus(&s->physical_sector_size); > + > + /* both block_size and sector_size are guaranteed powers of 2 */ > + s->sectors_per_block = s->params.block_size / s->logical_sector_size; Divide-by-zero if file is corrupt/malicious. > + s->chunk_ratio = (VHDX_MAX_SECTORS_PER_BLOCK) * > + (uint64_t)s->logical_sector_size / > + (uint64_t)s->params.block_size; Divide-by-zero if file is corrupt/malicious. > + > + > + /* These values are ones we will want to use for division / > multiplication > + * later on, and they are all guaranteed (per the spec) to be powers of > 2, > + * so we can take advantage of that for shift operations during > + * reads/writes */ Before doing that, let's verify that they are powers of two and fail if they violate the spec. if (x & x - 1) { ret = -EINVAL; goto exit; } > +static int vhdx_open(BlockDriverState *bs, int flags) > +{ > + BDRVVHDXState *s = bs->opaque; > + int ret = 0; > + int i; > + > + s->bat = NULL; > + > + qemu_co_mutex_init(&s->lock); > + > + ret = vhdx_parse_header(bs, s); > + if (ret) { > + goto fail; > + } > + > + ret = vhdx_parse_log(bs, s); > + if (ret) { > + goto fail; > + } > + > + ret = vhdx_open_region_tables(bs, s); > + if (ret) { > + goto fail; > + } > + > + ret = vhdx_parse_metadata(bs, s); > + if (ret) { > + goto fail; > + } > + s->block_size = s->params.block_size; > + > + /* the VHDX spec dictates that virtual_disk_size is always a multiple of > + * logical_sector_size */ > + bs->total_sectors = s->virtual_disk_size / s->logical_sector_size; > + > + s->bat_offset = s->bat_rt.file_offset; > + s->bat_entries = s->bat_rt.length / sizeof(vhdx_bat_entry); > + s->bat = g_malloc(s->bat_rt.length); > + > + ret = bdrv_pread(bs->file, s->bat_offset, s->bat, s->bat_rt.length); > + > + for (i = 0; i < s->bat_entries; i++) { > + le64_to_cpus(&s->bat[i]); > + } How does BAT size scale when the image size is increased? QCOW2 and QED use caches for metadata that would be too large or wasteful to keep in memory.