[Qemu-devel] [PATCH] qemu-img: avoid overflow of min_sparse parameter

[Qemu-devel] [PATCH] qemu-img: avoid overflow of min_sparse parameter

[Peter Lieven]

the min_sparse convert parameter can overflow (e.g. -S 1024G)
in the conversion from int64_t to int resulting in a negative
min_sparse parameter. Avoid this by limiting the valid parameters
to sane values. In fact anything exceeding the convert buffer size
is also pointless. While at it also forbid values that are non
multiple of 512 to avoid undesired behaviour. Values between 1 and
511 were legal, but resulted in full allocation.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 4a7ce43..2896746 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2005,6 +2005,8 @@ static int convert_do_copy(ImgConvertState *s)
     return s->ret;
 }
 
+#define MAX_BUF_SECTORS 32768
+
 static int img_convert(int argc, char **argv)
 {
     int c, bs_i, flags, src_flags = 0;
@@ -2100,8 +2102,12 @@ static int img_convert(int argc, char **argv)
             int64_t sval;
 
             sval = cvtnum(optarg);
-            if (sval < 0) {
-                error_report("Invalid minimum zero buffer size for sparse output specified");
+            if (sval < 0 || sval & BDRV_SECTOR_BITS ||
+                sval / BDRV_SECTOR_SIZE > MAX_BUF_SECTORS) {
+                error_report("Invalid buffer size for sparse output specified. "
+                             "Valid sizes are multiples of 512 up to %d. Select "
+                             "0 to disable sparse detection (fully allocates output).",
+                             MAX_BUF_SECTORS * 512);
                 goto fail_getopt;
             }
 
@@ -2385,9 +2391,9 @@ static int img_convert(int argc, char **argv)
     }
 
     /* increase bufsectors from the default 4096 (2M) if opt_transfer
-     * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
-     * as maximum. */
-    s.buf_sectors = MIN(32768,
+     * or discard_alignment of the out_bs is greater. Limit to
+     * MAX_BUF_SECTORS as maximum which is currently 32768 (16MB). */
+    s.buf_sectors = MIN(MAX_BUF_SECTORS,
                         MAX(s.buf_sectors,
                             MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS,
                                 out_bs->bl.pdiscard_alignment >>
-- 
2.7.4

Re: [Qemu-devel] [PATCH] qemu-img: avoid overflow of min_sparse parameter

[Kevin Wolf]

Am 12.07.2018 um 21:08 hat Peter Lieven geschrieben:
> the min_sparse convert parameter can overflow (e.g. -S 1024G)
> in the conversion from int64_t to int resulting in a negative
> min_sparse parameter. Avoid this by limiting the valid parameters
> to sane values. In fact anything exceeding the convert buffer size
> is also pointless. While at it also forbid values that are non
> multiple of 512 to avoid undesired behaviour. Values between 1 and
> 511 were legal, but resulted in full allocation.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4a7ce43..2896746 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2005,6 +2005,8 @@ static int convert_do_copy(ImgConvertState *s)
>      return s->ret;
>  }
>  
> +#define MAX_BUF_SECTORS 32768
> +
>  static int img_convert(int argc, char **argv)
>  {
>      int c, bs_i, flags, src_flags = 0;
> @@ -2100,8 +2102,12 @@ static int img_convert(int argc, char **argv)
>              int64_t sval;
>  
>              sval = cvtnum(optarg);
> -            if (sval < 0) {
> -                error_report("Invalid minimum zero buffer size for sparse output specified");
> +            if (sval < 0 || sval & BDRV_SECTOR_BITS ||

BDRV_SECTOR_BITS is 9 (because 1 << 9 == BDRV_SECTOR_SIZE), not a bit
mask to be used with &. I think what you want is BDRV_SECTOR_SIZE - 1.

Kevin