Re: [PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() error path

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 13/1/23 11:45, Kevin Wolf wrote:
> Am 13.01.2023 um 08:30 hat Philippe Mathieu-Daudé geschrieben:
>> On 12/1/23 20:14, Kevin Wolf wrote:
>>> In order to write the bitmap table to the image file, it is converted to
>>> big endian. If the write fails, it is passed to clear_bitmap_table() to
>>> free all of the clusters it had allocated before. However, if we don't
>>> convert it back to native endianness first, we'll free things at a wrong
>>> offset.
>>>
>>> In practical terms, the offsets will be so high that we won't actually
>>> free any allocated clusters, but just run into an error, but in theory
>>> this can cause image corruption.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    block/qcow2-bitmap.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)

>> Maybe add a comment here remembering to bswap back to native endianness?
>>
>>> -static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size)
>>> +static inline void bitmap_table_bswap_be(uint64_t *bitmap_table, size_t size)
>>>    {
>>
>> This function uses cpu_to_be64(), semantically we convert back calling
>> be64_to_cpu(), but technically both functions end up being the same.
> 
> Yes, but we don't seem to have any public "neutral" functions, it's
> always either from or to.
> 
>> Alternatively:
>>
>>       for (i = 0; i < size; ++i) {
>> -        bitmap_table[i] = cpu_to_be64(bitmap_table[i]);
>> +        bswap64s(&bitmap_table[i]);
>>       }
> 
> Doesn't that swap even on big endian hosts, resulting incorrectly in a
> little endian table?

Oops yes you are right... sorry!

> The closest thing we have that I can see is the be_bswap() macro in
> bswap.h, but it's undefined again at the end of the header.

Indeed.

Regards,

Phil.