Re: [Qemu-block] [Qemu-devel] [PATCH v2] vmdk: return ERROR when cluster sector is larger than vmdk limitation

[Eric Blake]

On 03/22/2018 02:41 AM, Fam Zheng wrote:
> On Thu, 03/22 14:24, address@hidden wrote:
> > From: yuchenlin <address@hidden>
> >
> > VMDK has a hard limitation of extent size, which is due to the size of grain
> > table entry is 32 bits. It means it can only point to a grain located at
> > offset = 2^32. To avoid writing the user data beyond limitation and record a 
> > useless offset
> > in grain table. We should return ERROR here.
> >
> > Signed-off-by: yuchenlin <address@hidden>
> > ---

> > +++ b/block/vmdk.c
> > @@ -47,6 +47,8 @@
> >   #define VMDK4_FLAG_MARKER (1 << 17)
> >   #define VMDK4_GD_AT_END 0xffffffffffffffffULL
> >   
> > +#define VMDK_EXTENT_MAX_SECTORS (4294967296)
> > +

Hmm, with sectors instead of bytes, you don't want to use Phillipe's '2 
* T_BYTE' macro after all; but it would still look better spelled as 
'(1ULL << 32)' or '(4ULL * 1024 * 1024 * 1024)'.  Most importantly, 
since the value does NOT fit in a 32-bit unsigned integer, I think you 
NEED to use the ULL suffix to make your intent clear.

> >   #define VMDK_GTE_ZEROED 0x1
> >   
> >   /* VMDK internal error codes */
> > @@ -1250,6 +1252,10 @@ static int get_cluster_offset(BlockDriverState *bs,
> >               return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
> >           }
> >   
> > +        if (extent->next_cluster_sector >= VMDK_EXTENT_MAX_SECTORS) {
> > +            return VMDK_ERROR;
> > +        }
> > +
> >           cluster_sector = extent->next_cluster_sector;
> >           extent->next_cluster_sector += extent->cluster_sectors;
> >   
> > --
> > 2.16.2
>
> Cc'ing Kevin and Max who merge block/ patches.

Also, CC'ing qemu-block, as all block-related patches should go through 
there.

> Reviewed-by: Fam Zheng <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org