On 19.08.19 21:34, Eric Blake wrote:
> 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))

As I said in the last version, I have this assertion here just because
of the following bdrv_pwrite(); so all we need to assert is that it fits
BDRV_REQUEST_MAX_BYTES (which it clearly does, as you say).

Max