| Message ID | 1395869885-27751-2-git-send-email-sw@weilnetz.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 26, 2014 at 10:38:05PM +0100, Stefan Weil wrote: > (1) block_size must not be null. > > (2) blocks_in_image * 4 must fit into a size_t. > > (3) blocks_in_image * block_size must fit into a uint64_t. > > Header field disk_size already has a bounds check which now works > because of modification (1) and (3). > > This patch was inspired by Jeff Cody's patch for the same problem. > > Cc: Jeff Cody <jcody@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > Hello Stefan, hello Jeff, > > I tried to improve your previous patch - maybe you want to improve it further > or take parts from it to fix that CVE (of which I have no information). > Thanks! > The patch was compiled on 32 and 64 bit Linux and cross compiled with MinGW-w64, > but not tested otherwise. > > Regards > Stefan > > block/vdi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 45 insertions(+), 5 deletions(-) > > diff --git a/block/vdi.c b/block/vdi.c > index b832905..8fb59a1 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -420,7 +420,12 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > header.sector_size, SECTOR_SIZE); > ret = -ENOTSUP; > goto fail; > -#if !defined(CONFIG_VDI_BLOCK_SIZE) Is this from qemu/master? I don't see the above in the master branch. > +#if defined(CONFIG_VDI_BLOCK_SIZE) > + } else if (header.block_size == 0) { > + error_setg(errp, "unsupported VDI image (block size is null)"); > + ret = -EINVAL; > + goto fail; > +#else The above is essentially dead code, since CONFIG_VDI_BLOCK_SIZE is commented out. Should this be added to configure, so that it can be compiled in like other options? I think if it is worth putting the code in the driver, it probably makes sense to have a configure option for it. Otherwise, in my opinion, it is generally best to excise dead code. With regards to adding this prior to QEMU 2.0, this would be more in line with a feature addition, I think (i.e., the support of non-default block sizes). > } else if (header.block_size != DEFAULT_CLUSTER_SIZE) { > error_setg(errp, "unsupported VDI image (block size %u is not %u)", > header.block_size, DEFAULT_CLUSTER_SIZE); > @@ -435,6 +440,18 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > (uint64_t)header.blocks_in_image * header.block_size); > ret = -ENOTSUP; > goto fail; > +#if SIZE_MAX <= UINT32_MAX > + } else if (header.blocks_in_image > SIZE_MAX / sizeof(uint32_t)) { > + /* blocks_in_image * sizeof(uint32_t) must fit into size_t. */ > + error_setg(errp, "unsupported VDI image (number of blocks %u, " > + "only %zu are possible)", > + header.blocks_in_image, SIZE_MAX / sizeof(uint32_t)); > +#endif > + } else if (header.blocks_in_image > UINT64_MAX / header.block_size) { > + /* blocks_in_image * block_size must fit into uint64_t. */ > + error_setg(errp, "unsupported VDI image (number of blocks %u, " > + "only %" PRIu64 " are possible)", > + header.blocks_in_image, UINT64_MAX / header.block_size); Since header.blocks_in_image is a uint32_t, this means the driver may allocate up to 16GB of ram... that is my main concern here. > } else if (!uuid_is_null(header.uuid_link)) { > error_setg(errp, "unsupported VDI image (non-NULL link UUID)"); > ret = -ENOTSUP; > @@ -691,6 +708,33 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options, > options++; > } > > +#if defined(CONFIG_VDI_BLOCK_SIZE) > + if (block_size == 0) { > + return -EINVAL; > + } > +#endif > + Same concern as above with regards to dead code / new feature > + if (bytes > UINT64_MAX - block_size) { > + /* Overflow in calculation of blocks (see below). */ > + return -ENOTSUP; > + } > + > + /* We need enough blocks to store the given disk size, > + so always round up. */ > + blocks = (bytes + block_size - 1) / block_size; > + > +#if SIZE_MAX <= UINT32_MAX > + if (blocks > SIZE_MAX / sizeof(uint32_t)) { > + /* blocks * sizeof(uint32_t) must fit into size_t. */ > + return -ENOTSUP; > + } > +#endif > + > + if (blocks > UINT64_MAX / block_size) { > + /* blocks * block_size must fit into uint64_t. */ > + return -EINVAL; > + } > + > fd = qemu_open(filename, > O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, > 0644); > @@ -698,10 +742,6 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options, > return -errno; > } > > - /* We need enough blocks to store the given disk size, > - so always round up. */ > - blocks = (bytes + block_size - 1) / block_size; > - > bmap_size = blocks * sizeof(uint32_t); > bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1)); > > -- > 1.7.10.4 >
Am 27.03.2014 20:49, schrieb Jeff Cody: > On Wed, Mar 26, 2014 at 10:38:05PM +0100, Stefan Weil wrote: >> (1) block_size must not be null. >> >> (2) blocks_in_image * 4 must fit into a size_t. >> >> (3) blocks_in_image * block_size must fit into a uint64_t. >> >> Header field disk_size already has a bounds check which now works >> because of modification (1) and (3). >> >> This patch was inspired by Jeff Cody's patch for the same problem. >> >> Cc: Jeff Cody <jcody@redhat.com> >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Stefan Weil <sw@weilnetz.de> >> --- >> >> Hello Stefan, hello Jeff, >> >> I tried to improve your previous patch - maybe you want to improve it further >> or take parts from it to fix that CVE (of which I have no information). >> > > Thanks! > >> The patch was compiled on 32 and 64 bit Linux and cross compiled with MinGW-w64, >> but not tested otherwise. >> >> Regards >> Stefan >> >> block/vdi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 45 insertions(+), 5 deletions(-) >> >> diff --git a/block/vdi.c b/block/vdi.c >> index b832905..8fb59a1 100644 >> --- a/block/vdi.c >> +++ b/block/vdi.c >> @@ -420,7 +420,12 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, >> header.sector_size, SECTOR_SIZE); >> ret = -ENOTSUP; >> goto fail; >> -#if !defined(CONFIG_VDI_BLOCK_SIZE) > > Is this from qemu/master? I don't see the above in the master > branch. It's from the previous patch (PATCH 1/2): http://patchwork.ozlabs.org/patch/334116/. You have to apply both patches in sequence. > >> +#if defined(CONFIG_VDI_BLOCK_SIZE) >> + } else if (header.block_size == 0) { >> + error_setg(errp, "unsupported VDI image (block size is null)"); >> + ret = -EINVAL; >> + goto fail; >> +#else > > The above is essentially dead code, since CONFIG_VDI_BLOCK_SIZE is > commented out. Should this be added to configure, so that it can be > compiled in like other options? I think if it is worth putting the > code in the driver, it probably makes sense to have a configure option > for it. Otherwise, in my opinion, it is generally best to excise dead > code. > > With regards to adding this prior to QEMU 2.0, this would be more in > line with a feature addition, I think (i.e., the support of > non-default block sizes). > Yes, it would be a very bad idea to activate variable block sizes for QEMU 2.0. I don't think adding a configure option makes sense here. If it's only for code coverage, you can pass a -DCONFIG_VDI_BLOCK_SIZE to the compiler. The main point is that I did not find any existing VDI image in the wild which uses a non default block size, nor were there any bug reports about that missing feature. As long as there are no hints that variable block sizes not only exist in theory but also in real live outside of QEMU, they should remain disabled in QEMU, too. >> } else if (header.block_size != DEFAULT_CLUSTER_SIZE) { >> error_setg(errp, "unsupported VDI image (block size %u is not %u)", >> header.block_size, DEFAULT_CLUSTER_SIZE); >> @@ -435,6 +440,18 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, >> (uint64_t)header.blocks_in_image * header.block_size); >> ret = -ENOTSUP; >> goto fail; >> +#if SIZE_MAX <= UINT32_MAX >> + } else if (header.blocks_in_image > SIZE_MAX / sizeof(uint32_t)) { >> + /* blocks_in_image * sizeof(uint32_t) must fit into size_t. */ >> + error_setg(errp, "unsupported VDI image (number of blocks %u, " >> + "only %zu are possible)", >> + header.blocks_in_image, SIZE_MAX / sizeof(uint32_t)); >> +#endif >> + } else if (header.blocks_in_image > UINT64_MAX / header.block_size) { >> + /* blocks_in_image * block_size must fit into uint64_t. */ >> + error_setg(errp, "unsupported VDI image (number of blocks %u, " >> + "only %" PRIu64 " are possible)", >> + header.blocks_in_image, UINT64_MAX / header.block_size); > > Since header.blocks_in_image is a uint32_t, this means the driver may > allocate up to 16GB of ram... that is my main concern here. > People who want to run disk images with hundreds of terabyte should be able to afford 16 GB of RAM :-) And if that memory is not available, QEMU will tell them so (the memory allocators terminate the process in this case). > >> } else if (!uuid_is_null(header.uuid_link)) { >> error_setg(errp, "unsupported VDI image (non-NULL link UUID)"); >> ret = -ENOTSUP; >> @@ -691,6 +708,33 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options, >> options++; >> } >> >> +#if defined(CONFIG_VDI_BLOCK_SIZE) >> + if (block_size == 0) { >> + return -EINVAL; >> + } >> +#endif >> + > > Same concern as above with regards to dead code / new feature > Activating that code would not be a good idea without prior tests and existing real live examples. Otherwise we might create buggy images or flood the world with incompatible VDI files. Removing that optional code would make it harder if somebody needs it: he or she would have to reinvent the wheel. Yes, we have a dilemma here. Regards Stefan
Stefan Weil <sw@weilnetz.de> writes: > Am 27.03.2014 20:49, schrieb Jeff Cody: >> On Wed, Mar 26, 2014 at 10:38:05PM +0100, Stefan Weil wrote: >>> (1) block_size must not be null. >>> >>> (2) blocks_in_image * 4 must fit into a size_t. >>> >>> (3) blocks_in_image * block_size must fit into a uint64_t. >>> >>> Header field disk_size already has a bounds check which now works >>> because of modification (1) and (3). >>> >>> This patch was inspired by Jeff Cody's patch for the same problem. >>> >>> Cc: Jeff Cody <jcody@redhat.com> >>> Cc: Kevin Wolf <kwolf@redhat.com> >>> Cc: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: Stefan Weil <sw@weilnetz.de> >>> --- >>> >>> Hello Stefan, hello Jeff, >>> >>> I tried to improve your previous patch - maybe you want to improve it further >>> or take parts from it to fix that CVE (of which I have no information). >>> >> >> Thanks! >> >>> The patch was compiled on 32 and 64 bit Linux and cross compiled with MinGW-w64, >>> but not tested otherwise. >>> >>> Regards >>> Stefan >>> >>> block/vdi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- >>> 1 file changed, 45 insertions(+), 5 deletions(-) >>> >>> diff --git a/block/vdi.c b/block/vdi.c >>> index b832905..8fb59a1 100644 >>> --- a/block/vdi.c >>> +++ b/block/vdi.c >>> @@ -420,7 +420,12 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, >>> header.sector_size, SECTOR_SIZE); >>> ret = -ENOTSUP; >>> goto fail; >>> -#if !defined(CONFIG_VDI_BLOCK_SIZE) >> >> Is this from qemu/master? I don't see the above in the master >> branch. > > It's from the previous patch (PATCH 1/2): > http://patchwork.ozlabs.org/patch/334116/. You have to apply both > patches in sequence. > >> >>> +#if defined(CONFIG_VDI_BLOCK_SIZE) >>> + } else if (header.block_size == 0) { >>> + error_setg(errp, "unsupported VDI image (block size is null)"); >>> + ret = -EINVAL; >>> + goto fail; >>> +#else >> >> The above is essentially dead code, since CONFIG_VDI_BLOCK_SIZE is >> commented out. Should this be added to configure, so that it can be >> compiled in like other options? I think if it is worth putting the >> code in the driver, it probably makes sense to have a configure option >> for it. Otherwise, in my opinion, it is generally best to excise dead >> code. >> >> With regards to adding this prior to QEMU 2.0, this would be more in >> line with a feature addition, I think (i.e., the support of >> non-default block sizes). >> > > Yes, it would be a very bad idea to activate variable block sizes for > QEMU 2.0. I don't think adding a configure option makes sense here. If > it's only for code coverage, you can pass a -DCONFIG_VDI_BLOCK_SIZE to > the compiler. The main point is that I did not find any existing VDI > image in the wild which uses a non default block size, nor were there > any bug reports about that missing feature. As long as there are no > hints that variable block sizes not only exist in theory but also in > real live outside of QEMU, they should remain disabled in QEMU, too. > > >>> } else if (header.block_size != DEFAULT_CLUSTER_SIZE) { >>> error_setg(errp, "unsupported VDI image (block size %u is not %u)", >>> header.block_size, DEFAULT_CLUSTER_SIZE); >>> @@ -435,6 +440,18 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, >>> (uint64_t)header.blocks_in_image * header.block_size); >>> ret = -ENOTSUP; >>> goto fail; >>> +#if SIZE_MAX <= UINT32_MAX >>> + } else if (header.blocks_in_image > SIZE_MAX / sizeof(uint32_t)) { >>> + /* blocks_in_image * sizeof(uint32_t) must fit into size_t. */ >>> + error_setg(errp, "unsupported VDI image (number of blocks %u, " >>> + "only %zu are possible)", >>> + header.blocks_in_image, SIZE_MAX / sizeof(uint32_t)); >>> +#endif >>> + } else if (header.blocks_in_image > UINT64_MAX / header.block_size) { >>> + /* blocks_in_image * block_size must fit into uint64_t. */ >>> + error_setg(errp, "unsupported VDI image (number of blocks %u, " >>> + "only %" PRIu64 " are possible)", >>> + header.blocks_in_image, UINT64_MAX / header.block_size); >> >> Since header.blocks_in_image is a uint32_t, this means the driver may >> allocate up to 16GB of ram... that is my main concern here. >> > > People who want to run disk images with hundreds of terabyte should be > able to afford 16 GB of RAM :-) And if that memory is not available, > QEMU will tell them so (the memory allocators terminate the process in > this case). Hot plug the wrong image, kill your guest. Not nice. Suggest to use a non-aborting memory allocation function for allocations whose size comes from the user. Precedence: we do that for guest RAM even though it's not hot-plug, just to get a decent error message. >> >>> } else if (!uuid_is_null(header.uuid_link)) { >>> error_setg(errp, "unsupported VDI image (non-NULL link UUID)"); >>> ret = -ENOTSUP; >>> @@ -691,6 +708,33 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options, >>> options++; >>> } >>> >>> +#if defined(CONFIG_VDI_BLOCK_SIZE) >>> + if (block_size == 0) { >>> + return -EINVAL; >>> + } >>> +#endif >>> + >> >> Same concern as above with regards to dead code / new feature >> > > Activating that code would not be a good idea without prior tests and > existing real live examples. Otherwise we might create buggy images or > flood the world with incompatible VDI files. > > Removing that optional code would make it harder if somebody needs it: > he or she would have to reinvent the wheel. > > Yes, we have a dilemma here. Code under #if 0 invariably rots. If it's an untested sketch like here, it starts life already rotten. Trap for the unwary. If you want to sketch some future feature in the code, please mark it clearly, so nobody gets tempted to treat it as anything but a sketch.
diff --git a/block/vdi.c b/block/vdi.c index b832905..8fb59a1 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -420,7 +420,12 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, header.sector_size, SECTOR_SIZE); ret = -ENOTSUP; goto fail; -#if !defined(CONFIG_VDI_BLOCK_SIZE) +#if defined(CONFIG_VDI_BLOCK_SIZE) + } else if (header.block_size == 0) { + error_setg(errp, "unsupported VDI image (block size is null)"); + ret = -EINVAL; + goto fail; +#else } else if (header.block_size != DEFAULT_CLUSTER_SIZE) { error_setg(errp, "unsupported VDI image (block size %u is not %u)", header.block_size, DEFAULT_CLUSTER_SIZE); @@ -435,6 +440,18 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, (uint64_t)header.blocks_in_image * header.block_size); ret = -ENOTSUP; goto fail; +#if SIZE_MAX <= UINT32_MAX + } else if (header.blocks_in_image > SIZE_MAX / sizeof(uint32_t)) { + /* blocks_in_image * sizeof(uint32_t) must fit into size_t. */ + error_setg(errp, "unsupported VDI image (number of blocks %u, " + "only %zu are possible)", + header.blocks_in_image, SIZE_MAX / sizeof(uint32_t)); +#endif + } else if (header.blocks_in_image > UINT64_MAX / header.block_size) { + /* blocks_in_image * block_size must fit into uint64_t. */ + error_setg(errp, "unsupported VDI image (number of blocks %u, " + "only %" PRIu64 " are possible)", + header.blocks_in_image, UINT64_MAX / header.block_size); } else if (!uuid_is_null(header.uuid_link)) { error_setg(errp, "unsupported VDI image (non-NULL link UUID)"); ret = -ENOTSUP; @@ -691,6 +708,33 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options, options++; } +#if defined(CONFIG_VDI_BLOCK_SIZE) + if (block_size == 0) { + return -EINVAL; + } +#endif + + if (bytes > UINT64_MAX - block_size) { + /* Overflow in calculation of blocks (see below). */ + return -ENOTSUP; + } + + /* We need enough blocks to store the given disk size, + so always round up. */ + blocks = (bytes + block_size - 1) / block_size; + +#if SIZE_MAX <= UINT32_MAX + if (blocks > SIZE_MAX / sizeof(uint32_t)) { + /* blocks * sizeof(uint32_t) must fit into size_t. */ + return -ENOTSUP; + } +#endif + + if (blocks > UINT64_MAX / block_size) { + /* blocks * block_size must fit into uint64_t. */ + return -EINVAL; + } + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, 0644); @@ -698,10 +742,6 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options, return -errno; } - /* We need enough blocks to store the given disk size, - so always round up. */ - blocks = (bytes + block_size - 1) / block_size; - bmap_size = blocks * sizeof(uint32_t); bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1));
(1) block_size must not be null. (2) blocks_in_image * 4 must fit into a size_t. (3) blocks_in_image * block_size must fit into a uint64_t. Header field disk_size already has a bounds check which now works because of modification (1) and (3). This patch was inspired by Jeff Cody's patch for the same problem. Cc: Jeff Cody <jcody@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefan Weil <sw@weilnetz.de> --- Hello Stefan, hello Jeff, I tried to improve your previous patch - maybe you want to improve it further or take parts from it to fix that CVE (of which I have no information). The patch was compiled on 32 and 64 bit Linux and cross compiled with MinGW-w64, but not tested otherwise. Regards Stefan block/vdi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-)