On 8/19/19 1:55 PM, Max Reitz wrote: > The qcow2 specification says to ignore unknown extra data fields in > snapshot table entries. Currently, we discard it whenever we update the > image, which is a bit different from "ignore". > > This patch makes the qcow2 driver keep all unknown extra data fields > when updating an image's snapshot table. > > @@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp) > sn->date_sec = be32_to_cpu(h.date_sec); > sn->date_nsec = be32_to_cpu(h.date_nsec); > sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec); > - extra_data_size = be32_to_cpu(h.extra_data_size); > + sn->extra_data_size = be32_to_cpu(h.extra_data_size); > > id_str_size = be16_to_cpu(h.id_str_size); > name_size = be16_to_cpu(h.name_size); > > - /* Read extra data */ > + if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) { > + ret = -EFBIG; > + error_setg(errp, "Too much extra metadata in snapshot table " > + "entry %i", i); > + goto fail; We fail if extra_data_size is > 1024... > + if (sn->extra_data_size > sizeof(extra)) { > + /* Store unknown extra data */ > + size_t unknown_extra_data_size = > + sn->extra_data_size - sizeof(extra); > + But read at most 1008 bytes into sn->unknown_extra_data. > @@ -234,6 +257,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs) > } > offset += sizeof(extra); > > + if (sn->extra_data_size > sizeof(extra)) { > + size_t unknown_extra_data_size = > + sn->extra_data_size - sizeof(extra); > + > + /* qcow2_read_snapshots() ensures no unbounded allocation */ > + assert(unknown_extra_data_size <= BDRV_REQUEST_MAX_BYTES); So this assertion is quite loose in what it permits; tighter would be assert(unknown_extra_data_size <= QCOW_MAX_SNAPSHOT_EXTRA_DATA - sizeof(extra)) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org