qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()
Date: Tue, 14 Nov 2017 16:40:55 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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 <address@hidden>
> 
> Berto
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]