Re: [PATCH for-8.0] block/vhdx: fix dynamic VHDX BAT corruption

[Kevin Wolf]

Am 11.04.2023 um 13:08 hat Kevin Wolf geschrieben:
> Am 08.04.2023 um 00:11 hat Lukas Tschoke geschrieben:
> > The corruption occurs when a BAT entry aligned to 4096 bytes is changed.
> > 
> > Specifically, the corruption occurs during the creation of the LOG Data
> > Descriptor. The incorrect behavior involves copying 4088 bytes from the
> > original 4096 bytes aligned offset to `tmp[8..4096]` and then copying
> > the new value for the first BAT entry to the beginning `tmp[0..8]`.
> > This results in all existing BAT entries inside the 4K region being
> > incorrectly moved by 8 bytes and the last entry being lost.
> > 
> > This bug did not cause noticeable corruption when only sequentially
> > writing once to an empty dynamic VHDX (e.g.
> > using `qemu-img convert -O vhdx -o subformat=dynamic ...`), but it
> > still resulted in invalid values for the (unused) Sector Bitmap BAT
> > entries.
> > 
> > Importantly, this corruption would only become noticeable after the
> > corrupted BAT is re-read from the file.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/727
> > Signed-off-by: Lukas Tschoke <lukts330@gmail.com>
> 
> Thanks, applied to the block branch.
> 
> Reviewing this function was a bit confusing, but after understanding
> what each variable means, your change is clearly fixing a local bug and
> therefore an improvement.
> 
> But now I'm wondering if it's really right that we don't have to handle
> the case where we write only a few bytes and therefore can have a
> leading and a trailing part in the same log sector.
> 
> In fact, having everything in the same sector actually seems to be the
> only case that really happens because vhdx_log_write_and_flush() is only
> called with length = 8 and offset = bat_entry_offset, which is a
> multiple of 8.
> 
> Most of the cases should be in the middle of the BAT. vhdx_log_write()
> uses the leading_length != 0 code path for them. This reads the part
> before the written entry from the image, replaces the entry itself, but
> seems to leave the buffer uninitialised for everything after the entry.
> So does that part get corrupted?

Ah, never mind. It actually reads the full sector and then overwrites
part of it. So this should be fine.

Kevin