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

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

[Thomas Huth]

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."

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) {
+            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);
-- 
2.56.0

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

[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.

>
> +            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.

Kevin

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

[Thomas Huth]

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...)

>>
>> +            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.

  Thomas

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

[Kevin Wolf]

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

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

[Thomas Huth]

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."

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>
---
 block/vhdx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/vhdx.c b/block/vhdx.c
index 8013a90a1a4..f5beace56f5 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 ((rt_entry.file_offset & (MiB - 1)) ||
+            (rt_entry.length & (MiB - 1)) || rt_entry.length < MiB) {
+            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);
-- 
2.56.0

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

[Eric Blake]

On Wed, Nov 11, 2026 at 05:14:42PM +0100, Thomas Huth wrote:
> 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."
> 
> 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>
> ---
>  block/vhdx.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 8013a90a1a4..f5beace56f5 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 ((rt_entry.file_offset & (MiB - 1)) ||
> +            (rt_entry.length & (MiB - 1)) || rt_entry.length < MiB) {

Would QEMU_IS_ALIGNED() be better than open-coding the math here?
Also, the 'rt_entry.length < MiB' clause could be simplified to
'rt_entry.length == 0', since all other values less than MiB fail the
earlier alignment clause.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[Thomas Huth]

Am 11.11.26 um 21:32 schrieb Eric Blake:
> On Wed, Nov 11, 2026 at 05:14:42PM +0100, Thomas Huth wrote:
>> 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."
>>
>> 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>
>> ---
>>   block/vhdx.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/block/vhdx.c b/block/vhdx.c
>> index 8013a90a1a4..f5beace56f5 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 ((rt_entry.file_offset & (MiB - 1)) ||
>> +            (rt_entry.length & (MiB - 1)) || rt_entry.length < MiB) {
> 
> Would QEMU_IS_ALIGNED() be better than open-coding the math here?
That looks nicer, indeed. I'll send a v2...

  Thanks,
   Thomas