| Message ID | 6cfb6d6b-adc5-7772-c8a5-6bae9a0ad668@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/vhdx: fix dynamic VHDX BAT corruption | expand |
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? I suspect that while your patch fixes some cases, the function still isn't entirely right. Do you happen to have a simple reproducer using only qemu-io and qemu-img for the case you fixed? If so, maybe it could be relatively easily adjusted to cover these other cases, too. Kevin
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
diff --git a/block/vhdx-log.c b/block/vhdx-log.c index c48cf65d62..38148f107a 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -981,7 +981,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s, sector_write = merged_sector; } else if (i == sectors - 1 && trailing_length) { /* partial sector at the end of the buffer */ - ret = bdrv_pread(bs->file, file_offset, + ret = bdrv_pread(bs->file, file_offset + trailing_length, VHDX_LOG_SECTOR_SIZE - trailing_length, merged_sector + trailing_length, 0); if (ret < 0) {
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> --- block/vhdx-log.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)