Re: [PATCH] block/vhdx: Add alignment checks for region tables

[Kevin Wolf <kwolf@redhat.com>]

Am 12.11.2026 um 15:48 hat Thomas Huth geschrieben:
> Am 12.11.26 um 13:49 schrieb Kevin Wolf:
> > Am 12.11.2026 um 10:22 hat Thomas Huth geschrieben:
> > > According to the VHDX specification:
> > > 
> > >   "The FileOffset and Length fields specify the 64-bit byte offset and 32-bit
> > >    byte length of the object within the file. The values must be a multiple of
> > >    1 MB, and FileOffset must be at least 1 MB."
> > 
> > This requires that the offset (not length) is at least 1 MB...
> > 
> > > Thus let's add a sanity check to avoid that bad things could happen later.
> > > 
> > > Buglink: https://gitlab.com/qemu-project/qemu/-/work_items/3762
> > > Reported-by: Daniel Wade <danjwade95@gmail.com>
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   v2: Use QEMU_IS_ALIGNED() as suggested by Eric
> > > 
> > >   block/vhdx.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > index 8013a90a1a4..21270988b5a 100644
> > > --- a/block/vhdx.c
> > > +++ b/block/vhdx.c
> > > @@ -566,6 +566,16 @@ vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
> > >           vhdx_region_entry_le_import(&rt_entry);
> > > +        /*
> > > +         * According to the VHDX spec, offset and length must be multiple
> > > +         * of 1 MiB and length must be at least 1 MiB.
> > > +         */
> > > +        if (!QEMU_IS_ALIGNED(rt_entry.file_offset, MiB) ||
> > > +            !QEMU_IS_ALIGNED(rt_entry.length, MiB) || !rt_entry.length) {
> > 
> > ...but the comment and code do the opposite and check only that the
> > length is at least 1 MB, leaving FileOffset (which was the actual
> > requirement) unchecked.
> 
> D'oh, stupid me! I was apparently sloppy when reading the spec, sorry!
> (though a zero length likely also does not make much sense...)

Yes, I wouldn't have complained about the extra check for length, but
checking the offset seems important.

> > > 
> > > +            ret = -EINVAL;
> > > +            goto fail;
> > > +        }
> > > +
> > >           /* check for region overlap between these entries, and any
> > >            * other memory regions in the file */
> > >           ret = vhdx_region_check(s, rt_entry.file_offset, rt_entry.length);
> > 
> > I'm also wondering if the check shouldn't be in vhdx_region_check()
> > instead, which we're already calling here.
> 
> I considered that, too, but finally decided against it since
> vhdx_region_check() seemed rather about overlapping regions... but if you
> prefer, I can also move it there.

There's a comment at vhdx_region_check() that could use an update then,
but the name vhdx_region_check() feels generic enough to me.

But this is not a requirement from my side, just a thought I had. If we
both had it, maybe that means something, but you decide.

Kevin