Patchwork [1/1] block/vpc.c: Detect too-large vpc file

login
register
mail settings
Submitter Serge E. Hallyn
Date July 25, 2011, 6:34 p.m.
Message ID <20110725183435.GA26649@hallyn.com>
Download mbox | patch
Permalink /patch/106736/
State New
Headers show

Comments

Serge E. Hallyn - July 25, 2011, 6:34 p.m.
VHD files technically can be up to 2Tb, but virtual pc is limited
to 127G.  Currently qemu-img refused to create vpc files > 127G,
but it is failing to return error when converting from a non-vpc
VHD file which is >127G.  It returns success, but creates a truncated
converted image.  Also, qemu-img info claims the vpc file is 127G
(and clean).

This patch detects a too-large vpc file and returns -EFBIG.  Without
this patch,
Kevin Wolf - July 26, 2011, 9:01 a.m.
Am 25.07.2011 20:34, schrieb Serge E. Hallyn:
> VHD files technically can be up to 2Tb, but virtual pc is limited
> to 127G.  Currently qemu-img refused to create vpc files > 127G,
> but it is failing to return error when converting from a non-vpc
> VHD file which is >127G.  It returns success, but creates a truncated
> converted image.  Also, qemu-img info claims the vpc file is 127G
> (and clean).
> 
> This patch detects a too-large vpc file and returns -EFBIG.  Without
> this patch,
> 
> =============================================================
> root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/140g-dynamic.vhd 
> image: /mnt/140g-dynamic.vhd
> file format: vpc
> virtual size: 127G (136899993600 bytes)
> disk size: 284K
> root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
> root@ip-10-38-123-242:~/qemu-fixed# echo $?
> 0
> root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/y
> image: /mnt/y
> file format: raw
> virtual size: 127G (136899993600 bytes)
> disk size: 0
> =============================================================
> 
> (The 140G image was truncated with no warning or error.)
> 
> With the patch, I get:
> 
> =============================================================
> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> qemu-img: Could not open '/mnt/140g-dynamic.vhd'
> =============================================================
> 
> See https://bugs.launchpad.net/qemu/+bug/814222 for details.
> 
> Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
>  block/vpc.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 56865da..fdd5236 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -156,6 +156,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
>      struct vhd_dyndisk_header* dyndisk_header;
>      uint8_t buf[HEADER_SIZE];
>      uint32_t checksum;
> +    int err = -1;
>  
>      if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
>          goto fail;
> @@ -176,6 +177,11 @@ static int vpc_open(BlockDriverState *bs, int flags)
>      bs->total_sectors = (int64_t)
>          be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
>  
> +    if (bs->total_sectors >= 65535 * 16 * 255) {
> +        err = -EFBIG;
> +        goto fail;
> +    }

I wonder why this works. If bs->total_sectors was right, shouldn't it
have converted the full 140 GB? I can't see where else we would limit it
to 127 GB, so what I had expected is that the CHS geometry stored in the
image header is already too small.

What is the CHS geometry of this 140 GB image?

Kevin
Serge E. Hallyn - July 26, 2011, 4:08 p.m.
Quoting Kevin Wolf (kwolf@redhat.com):
> Am 25.07.2011 20:34, schrieb Serge E. Hallyn:
> > VHD files technically can be up to 2Tb, but virtual pc is limited
> > to 127G.  Currently qemu-img refused to create vpc files > 127G,
> > but it is failing to return error when converting from a non-vpc
> > VHD file which is >127G.  It returns success, but creates a truncated
> > converted image.  Also, qemu-img info claims the vpc file is 127G
> > (and clean).
> > 
> > This patch detects a too-large vpc file and returns -EFBIG.  Without
> > this patch,
> > 
> > =============================================================
> > root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/140g-dynamic.vhd 
> > image: /mnt/140g-dynamic.vhd
> > file format: vpc
> > virtual size: 127G (136899993600 bytes)
> > disk size: 284K
> > root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
> > root@ip-10-38-123-242:~/qemu-fixed# echo $?
> > 0
> > root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/y
> > image: /mnt/y
> > file format: raw
> > virtual size: 127G (136899993600 bytes)
> > disk size: 0
> > =============================================================
> > 
> > (The 140G image was truncated with no warning or error.)
> > 
> > With the patch, I get:
> > 
> > =============================================================
> > root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
> > qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> > root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
> > qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
> > qemu-img: Could not open '/mnt/140g-dynamic.vhd'
> > =============================================================
> > 
> > See https://bugs.launchpad.net/qemu/+bug/814222 for details.
> > 
> > Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
> > ---
> >  block/vpc.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> > 
> > diff --git a/block/vpc.c b/block/vpc.c
> > index 56865da..fdd5236 100644
> > --- a/block/vpc.c
> > +++ b/block/vpc.c
> > @@ -156,6 +156,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
> >      struct vhd_dyndisk_header* dyndisk_header;
> >      uint8_t buf[HEADER_SIZE];
> >      uint32_t checksum;
> > +    int err = -1;
> >  
> >      if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
> >          goto fail;
> > @@ -176,6 +177,11 @@ static int vpc_open(BlockDriverState *bs, int flags)
> >      bs->total_sectors = (int64_t)
> >          be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
> >  
> > +    if (bs->total_sectors >= 65535 * 16 * 255) {
> > +        err = -EFBIG;
> > +        goto fail;
> > +    }
> 
> I wonder why this works. If bs->total_sectors was right, shouldn't it

bs->total_sectors is exactly 65535 * 16 * 255.  It never gets bigger.
So really the check could be

    if (bs->total_sectors == 65535 * 16 * 255) {

and it would still work.

> have converted the full 140 GB? I can't see where else we would limit it
> to 127 GB, so what I had expected is that the CHS geometry stored in the
> image header is already too small.

If I understand you correctly, what you're saying is exactly what I'm
finding.  total_sectors is not reflective of the true size.

I assume the true size is somewhere to be found :)  But I haven't found a
nice spec for these.

thanks,
-serge
Kevin Wolf - July 26, 2011, 4:20 p.m.
Am 26.07.2011 18:08, schrieb Serge E. Hallyn:
> Quoting Kevin Wolf (kwolf@redhat.com):
>> Am 25.07.2011 20:34, schrieb Serge E. Hallyn:
>>> VHD files technically can be up to 2Tb, but virtual pc is limited
>>> to 127G.  Currently qemu-img refused to create vpc files > 127G,
>>> but it is failing to return error when converting from a non-vpc
>>> VHD file which is >127G.  It returns success, but creates a truncated
>>> converted image.  Also, qemu-img info claims the vpc file is 127G
>>> (and clean).
>>>
>>> This patch detects a too-large vpc file and returns -EFBIG.  Without
>>> this patch,
>>>
>>> =============================================================
>>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/140g-dynamic.vhd 
>>> image: /mnt/140g-dynamic.vhd
>>> file format: vpc
>>> virtual size: 127G (136899993600 bytes)
>>> disk size: 284K
>>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
>>> root@ip-10-38-123-242:~/qemu-fixed# echo $?
>>> 0
>>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/y
>>> image: /mnt/y
>>> file format: raw
>>> virtual size: 127G (136899993600 bytes)
>>> disk size: 0
>>> =============================================================
>>>
>>> (The 140G image was truncated with no warning or error.)
>>>
>>> With the patch, I get:
>>>
>>> =============================================================
>>> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
>>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
>>> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
>>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
>>> qemu-img: Could not open '/mnt/140g-dynamic.vhd'
>>> =============================================================
>>>
>>> See https://bugs.launchpad.net/qemu/+bug/814222 for details.
>>>
>>> Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
>>> ---
>>>  block/vpc.c |    8 +++++++-
>>>  1 files changed, 7 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index 56865da..fdd5236 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -156,6 +156,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>>      struct vhd_dyndisk_header* dyndisk_header;
>>>      uint8_t buf[HEADER_SIZE];
>>>      uint32_t checksum;
>>> +    int err = -1;
>>>  
>>>      if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
>>>          goto fail;
>>> @@ -176,6 +177,11 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>>      bs->total_sectors = (int64_t)
>>>          be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
>>>  
>>> +    if (bs->total_sectors >= 65535 * 16 * 255) {
>>> +        err = -EFBIG;
>>> +        goto fail;
>>> +    }
>>
>> I wonder why this works. If bs->total_sectors was right, shouldn't it
> 
> bs->total_sectors is exactly 65535 * 16 * 255.  It never gets bigger.
> So really the check could be
> 
>     if (bs->total_sectors == 65535 * 16 * 255) {
> 
> and it would still work.

Oh, now I understand. I think this is a bit too restrictive as it
forbids the largest possible size.

>> have converted the full 140 GB? I can't see where else we would limit it
>> to 127 GB, so what I had expected is that the CHS geometry stored in the
>> image header is already too small.
> 
> If I understand you correctly, what you're saying is exactly what I'm
> finding.  total_sectors is not reflective of the true size.
> 
> I assume the true size is somewhere to be found :)  But I haven't found a
> nice spec for these.

I think that footer->size contains the real size. Maybe we should do
something like this:

if (footer->size > 65536 * 16 * 255 * 512) {
    bs->total_sectors = footer->size / 512;
} else {
    bs->total_sectors = (int64_t)
        be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl
}

That is, if the image is small enough that CHS can exactly represent it,
we use the CHS even if it differs from the real size (because this is
the same as Virtual PC does), but if it's bigger let's forget about CHS
switch to the real size.

Probably needs a comment, too.

Kevin

Patch

=============================================================
root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/140g-dynamic.vhd 
image: /mnt/140g-dynamic.vhd
file format: vpc
virtual size: 127G (136899993600 bytes)
disk size: 284K
root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
root@ip-10-38-123-242:~/qemu-fixed# echo $?
0
root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/y
image: /mnt/y
file format: raw
virtual size: 127G (136899993600 bytes)
disk size: 0
=============================================================

(The 140G image was truncated with no warning or error.)

With the patch, I get:

=============================================================
root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd 
qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y
qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large
qemu-img: Could not open '/mnt/140g-dynamic.vhd'
=============================================================

See https://bugs.launchpad.net/qemu/+bug/814222 for details.

Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
---
 block/vpc.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 56865da..fdd5236 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -156,6 +156,7 @@  static int vpc_open(BlockDriverState *bs, int flags)
     struct vhd_dyndisk_header* dyndisk_header;
     uint8_t buf[HEADER_SIZE];
     uint32_t checksum;
+    int err = -1;
 
     if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
         goto fail;
@@ -176,6 +177,11 @@  static int vpc_open(BlockDriverState *bs, int flags)
     bs->total_sectors = (int64_t)
         be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
 
+    if (bs->total_sectors >= 65535 * 16 * 255) {
+        err = -EFBIG;
+        goto fail;
+    }
+
     if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
             != HEADER_SIZE)
         goto fail;
@@ -222,7 +228,7 @@  static int vpc_open(BlockDriverState *bs, int flags)
 
     return 0;
  fail:
-    return -1;
+    return err;
 }
 
 /*