Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

On 2017-11-14 16:38, Alberto Garcia wrote:
> On Tue 14 Nov 2017 04:27:56 PM CET, Max Reitz wrote:
>>>> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
>>>> +{
>>>> +    BDRVQcow2State *s = bs->opaque;
>>>> +    uint32_t index = offset_to_reftable_index(s, offset);
>>>> +    int64_t covering_refblock_offset = 0;
>>>> +
>>>> +    if (index < s->refcount_table_size) {
>>>> +        covering_refblock_offset = s->refcount_table[index] & REFT_OFFSET_MASK;
>>>> +    }
>>>> +    if (!covering_refblock_offset) {
>>>> +        qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " is "
>>>> +                                "not covered by the refcount structures",
>>>> +                                offset);
>>>> +        return -EIO;
>>>> +    }
>>>> +
>>>> +    return covering_refblock_offset;
>>>> +}
>>>
>>> Isn't it simpler to do something like this instead?
>>>
>>>    if (index >= s->refcount_table_size) {
>>>        qcow2_signal_corruption(...);
>>>        return -EIO;
>>>    }
>>>    return s->refcount_table[index] & REFT_OFFSET_MASK;
>>
>> But that doesn't cover the case were s->refcount_table[index] &
>> REFT_OFFSET_MASK is 0.
> 
> Ah, you're right. That would be detected by the qcow2_cache_get() call
> though, but it's fine to do the check here as well.

After patch 5, yes, but it would lead to a failed assertion.

Before this series, there is no protection in place; the cache will
happily serve you any empty entry (because offset 0 is used for empty
entries) and pretend it's the correct block.

Only when you try to dirty it is when you run into problems (because
then you'll get an assertion failure).

Max

> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]