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

[Philippe Mathieu-Daudé]

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.