* [Qemu-devel] [PATCH 1/2] vdi: Fix error message and two more small code improvements @ 2014-03-26 21:38 Stefan Weil 2014-03-26 21:38 ` [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144) Stefan Weil 0 siblings, 1 reply; 5+ messages in thread From: Stefan Weil @ 2014-03-26 21:38 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Stefan Weil, Stefan Hajnoczi * Fix wrong error message (copy+paste bug from commit 5b7aa9b56d1bfc79916262f380c3fc7961becb50). * Replace the default cluster size of 1 * MiB by DEFAULT_CLUSTER_SIZE. * Don't check for the default cluster size if we support other sizes, too. Cc: Kevin Wolf <kwolf@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefan Weil <sw@weilnetz.de> --- block/vdi.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/block/vdi.c b/block/vdi.c index ac9a025..b832905 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -420,11 +420,13 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, header.sector_size, SECTOR_SIZE); ret = -ENOTSUP; goto fail; - } else if (header.block_size != 1 * MiB) { - error_setg(errp, "unsupported VDI image (sector size %u is not %u)", - header.block_size, 1 * MiB); +#if !defined(CONFIG_VDI_BLOCK_SIZE) + } 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); ret = -ENOTSUP; goto fail; +#endif } else if (header.disk_size > (uint64_t)header.blocks_in_image * header.block_size) { error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", " -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144) 2014-03-26 21:38 [Qemu-devel] [PATCH 1/2] vdi: Fix error message and two more small code improvements Stefan Weil @ 2014-03-26 21:38 ` Stefan Weil 2014-03-27 19:49 ` Jeff Cody 0 siblings, 1 reply; 5+ messages in thread From: Stefan Weil @ 2014-03-26 21:38 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Stefan Weil, Jeff Cody, Stefan Hajnoczi (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(-) 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.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144) 2014-03-26 21:38 ` [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144) Stefan Weil @ 2014-03-27 19:49 ` Jeff Cody 2014-03-27 20:25 ` Stefan Weil 0 siblings, 1 reply; 5+ messages in thread From: Jeff Cody @ 2014-03-27 19:49 UTC (permalink / raw) To: Stefan Weil; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144) 2014-03-27 19:49 ` Jeff Cody @ 2014-03-27 20:25 ` Stefan Weil 2014-03-28 8:47 ` Markus Armbruster 0 siblings, 1 reply; 5+ messages in thread From: Stefan Weil @ 2014-03-27 20:25 UTC (permalink / raw) To: Jeff Cody; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144) 2014-03-27 20:25 ` Stefan Weil @ 2014-03-28 8:47 ` Markus Armbruster 0 siblings, 0 replies; 5+ messages in thread From: Markus Armbruster @ 2014-03-28 8:47 UTC (permalink / raw) To: Stefan Weil; +Cc: Kevin Wolf, Jeff Cody, qemu-devel, Stefan Hajnoczi 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-28 8:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-26 21:38 [Qemu-devel] [PATCH 1/2] vdi: Fix error message and two more small code improvements Stefan Weil 2014-03-26 21:38 ` [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144) Stefan Weil 2014-03-27 19:49 ` Jeff Cody 2014-03-27 20:25 ` Stefan Weil 2014-03-28 8:47 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).