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

Kevin Wolf posted 4 patches 1 year, 4 months ago
[PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() error path
Posted by Kevin Wolf 1 year, 4 months ago
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(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index bcad567c0c..3dff99ba06 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -115,7 +115,7 @@ static int update_header_sync(BlockDriverState *bs)
     return bdrv_flush(bs->file->bs);
 }
 
-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)
 {
     size_t i;
 
@@ -1401,9 +1401,10 @@ static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
         goto fail;
     }
 
-    bitmap_table_to_be(tb, tb_size);
+    bitmap_table_bswap_be(tb, tb_size);
     ret = bdrv_pwrite(bs->file, tb_offset, tb_size * sizeof(tb[0]), tb, 0);
     if (ret < 0) {
+        bitmap_table_bswap_be(tb, tb_size);
         error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file",
                          bm_name);
         goto fail;
-- 
2.38.1
Re: [PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() error path
Posted by Philippe Mathieu-Daudé 1 year, 4 months ago
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(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index bcad567c0c..3dff99ba06 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -115,7 +115,7 @@ static int update_header_sync(BlockDriverState *bs)
>       return bdrv_flush(bs->file->bs);
>   }
>   

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.

Alternatively:

      for (i = 0; i < size; ++i) {
-        bitmap_table[i] = cpu_to_be64(bitmap_table[i]);
+        bswap64s(&bitmap_table[i]);
      }

> @@ -1401,9 +1401,10 @@ static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
>           goto fail;
>       }
>   
> -    bitmap_table_to_be(tb, tb_size);
> +    bitmap_table_bswap_be(tb, tb_size);
>       ret = bdrv_pwrite(bs->file, tb_offset, tb_size * sizeof(tb[0]), tb, 0);
>       if (ret < 0) {
> +        bitmap_table_bswap_be(tb, tb_size);
>           error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file",
>                            bm_name);
>           goto fail;

Pre-existing, but consider using g_autofree for 'tb'.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Re: [PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() error path
Posted by Kevin Wolf 1 year, 4 months ago
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(-)
> > 
> > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> > index bcad567c0c..3dff99ba06 100644
> > --- a/block/qcow2-bitmap.c
> > +++ b/block/qcow2-bitmap.c
> > @@ -115,7 +115,7 @@ static int update_header_sync(BlockDriverState *bs)
> >       return bdrv_flush(bs->file->bs);
> >   }
> 
> 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?

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.

> > @@ -1401,9 +1401,10 @@ static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
> >           goto fail;
> >       }
> > -    bitmap_table_to_be(tb, tb_size);
> > +    bitmap_table_bswap_be(tb, tb_size);
> >       ret = bdrv_pwrite(bs->file, tb_offset, tb_size * sizeof(tb[0]), tb, 0);
> >       if (ret < 0) {
> > +        bitmap_table_bswap_be(tb, tb_size);
> >           error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file",
> >                            bm_name);
> >           goto fail;
> 
> Pre-existing, but consider using g_autofree for 'tb'.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

Kevin
Re: [PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() error path
Posted by Philippe Mathieu-Daudé 1 year, 4 months ago
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.