| Message ID | 20200501131525.6745-1-berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: Avoid integer wraparound in qcow2_co_truncate() | expand |
On 5/1/20 8:15 AM, Alberto Garcia wrote: > After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is > extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are > zeroized. > > The code however does not detect correctly situations when the old and > the new end of the image are within the same cluster. The problem can > be reproduced with these steps: > > qemu-img create -f qcow2 backing.qcow2 1M > qemu-img create -f qcow2 -b backing.qcow2 top.qcow2 We should get in the habit of documenting -F qcow2 (I have a series, still awaiting review, that would warn if you don't). > qemu-img resize --shrink top.qcow2 520k > qemu-img resize top.qcow2 567k > > In the last step offset - zero_start causes an integer wraparound. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 2ba0b17c39..6d34d28c60 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -4234,6 +4234,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) { > uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size); > > + /* zero_start should not be after the new end of the image */ > + zero_start = MIN(zero_start, offset); > + So, using your numbers, pre-patch, we have zero_start = 0x90000 (0x82000 rounded up to 0x10000 alignment). post-patch, the new MIN() lowers it back to 0x8dc00 (the new size), which is unaligned. > /* > * Use zero clusters as much as we can. qcow2_cluster_zeroize() > * requires a cluster-aligned start. The end may be unaligned if it is * at the end of the image (which it is here). */ ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); pre-patch, it called zeroize(, 0x90000, 0xffffffffffffdc00, ) post-patch, it calls zeroize(, 0x8dc00, 0, ) Looking at qcow2_cluster_zeroize, we have: assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); which will now trigger. This patch is a good idea, but needs a v2.
On 5/1/20 12:12 PM, Eric Blake wrote: > On 5/1/20 8:15 AM, Alberto Garcia wrote: >> After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is >> extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are >> zeroized. >> >> The code however does not detect correctly situations when the old and >> the new end of the image are within the same cluster. The problem can >> be reproduced with these steps: >> >> qemu-img create -f qcow2 backing.qcow2 1M >> qemu-img create -f qcow2 -b backing.qcow2 top.qcow2 > > We should get in the habit of documenting -F qcow2 (I have a series, > still awaiting review, that would warn if you don't). > >> qemu-img resize --shrink top.qcow2 520k >> qemu-img resize top.qcow2 567k >> Since your reproducer triggers assertion failure, I suggest doing this instead: >> +++ b/block/qcow2.c >> @@ -4234,6 +4234,9 @@ static int coroutine_fn >> qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >> if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) { >> uint64_t zero_start = QEMU_ALIGN_UP(old_length, >> s->cluster_size); >> + /* zero_start should not be after the new end of the image */ >> + zero_start = MIN(zero_start, offset); >> + Drop this hunk (leave zero_start unchanged), and instead... > > So, using your numbers, pre-patch, we have zero_start = 0x90000 (0x82000 > rounded up to 0x10000 alignment). post-patch, the new MIN() lowers it > back to 0x8dc00 (the new size), which is unaligned. > >> /* >> * Use zero clusters as much as we can. qcow2_cluster_zeroize() >> * requires a cluster-aligned start. The end may be >> unaligned if it is > * at the end of the image (which it is here). > */ > ret = qcow2_cluster_zeroize(bs, zero_start, offset - > zero_start, 0); ...patch _this_ call to compute 'QEMU_ALIGN_UP(offset, s->cluster_size) - zero_start' for the length.
On Fri 01 May 2020 08:48:31 PM CEST, Eric Blake wrote: > Since your reproducer triggers assertion failure, I suggest doing this > instead: >>> +++ b/block/qcow2.c >>> @@ -4234,6 +4234,9 @@ static int coroutine_fn >>> qcow2_co_truncate(BlockDriverState *bs, int64_t offset, >>> if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) { >>> uint64_t zero_start = QEMU_ALIGN_UP(old_length, >>> s->cluster_size); >>> + /* zero_start should not be after the new end of the image */ >>> + zero_start = MIN(zero_start, offset); >>> + > > Drop this hunk (leave zero_start unchanged), and instead... > >> >> So, using your numbers, pre-patch, we have zero_start = 0x90000 (0x82000 >> rounded up to 0x10000 alignment). post-patch, the new MIN() lowers it >> back to 0x8dc00 (the new size), which is unaligned. >> >>> /* >>> * Use zero clusters as much as we can. qcow2_cluster_zeroize() >>> * requires a cluster-aligned start. The end may be >>> unaligned if it is >> * at the end of the image (which it is here). >> */ >> ret = qcow2_cluster_zeroize(bs, zero_start, offset - >> zero_start, 0); > > ...patch _this_ call to compute 'QEMU_ALIGN_UP(offset, s->cluster_size) > - zero_start' for the length. That would work, but then we would be writing zeroes beyond the end of the image (but still within the last cluster). The other solution is to keep my hunk and call qcow2_cluster_zeroize() only when offset > zero_start. Berto
On 5/4/20 8:47 AM, Alberto Garcia wrote: >> Drop this hunk (leave zero_start unchanged), and instead... >> >>> >>> So, using your numbers, pre-patch, we have zero_start = 0x90000 (0x82000 >>> rounded up to 0x10000 alignment). post-patch, the new MIN() lowers it >>> back to 0x8dc00 (the new size), which is unaligned. >>> >>>> /* >>>> * Use zero clusters as much as we can. qcow2_cluster_zeroize() >>>> * requires a cluster-aligned start. The end may be >>>> unaligned if it is >>> * at the end of the image (which it is here). >>> */ >>> ret = qcow2_cluster_zeroize(bs, zero_start, offset - >>> zero_start, 0); >> >> ...patch _this_ call to compute 'QEMU_ALIGN_UP(offset, s->cluster_size) >> - zero_start' for the length. > > That would work, but then we would be writing zeroes beyond the end of > the image (but still within the last cluster). > > The other solution is to keep my hunk and call qcow2_cluster_zeroize() > only when offset > zero_start. Yes, that would work, and probably less complicated.
diff --git a/block/qcow2.c b/block/qcow2.c index 2ba0b17c39..6d34d28c60 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -4234,6 +4234,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) { uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size); + /* zero_start should not be after the new end of the image */ + zero_start = MIN(zero_start, offset); + /* * Use zero clusters as much as we can. qcow2_cluster_zeroize() * requires a cluster-aligned start. The end may be unaligned if it is
After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are zeroized. The code however does not detect correctly situations when the old and the new end of the image are within the same cluster. The problem can be reproduced with these steps: qemu-img create -f qcow2 backing.qcow2 1M qemu-img create -f qcow2 -b backing.qcow2 top.qcow2 qemu-img resize --shrink top.qcow2 520k qemu-img resize top.qcow2 567k In the last step offset - zero_start causes an integer wraparound. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2.c | 3 +++ 1 file changed, 3 insertions(+)