[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() erro
From: |
Kevin Wolf |
Subject: |
Re: [PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() error path |
Date: |
Fri, 13 Jan 2023 11:45:31 +0100 |
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
- [PATCH 0/4] qemu-img: Fix exit code for errors closing the image, Kevin Wolf, 2023/01/12
- [PATCH 2/4] qemu-img commit: Report errors while closing the image, Kevin Wolf, 2023/01/12
- [PATCH 3/4] qemu-img bitmap: Report errors while closing the image, Kevin Wolf, 2023/01/12
- [PATCH 4/4] qemu-iotests: Test qemu-img bitmap/commit exit code on error, Kevin Wolf, 2023/01/12
- Re: [PATCH 0/4] qemu-img: Fix exit code for errors closing the image, Markus Armbruster, 2023/01/13
- Re: [PATCH 0/4] qemu-img: Fix exit code for errors closing the image, Hanna Czenczek, 2023/01/17