[PATCH 0/4] Fix qemu-img bench issues and improve checks

[PATCH 0/4] Fix qemu-img bench issues and improve checks

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

This series fixes several qemu-img crashes found during fuzzing.

The patch "qemu-img: fix division by zero in bench_cb() for zero-sized" 
was already submitted earlier:  
https://lore.kernel.org/qemu-devel/20250318101933.255617-1-gerben@altlinux.org/  

However, it has been included in this series because it 
has not yet been merged into master.  Without it, the series 
would conflict with this commit due to modifications 
in the same parts of the code.  

Denis Rastyogin (4):
  qemu-img: fix division by zero in bench_cb() for zero-sized
  qemu-img: fix offset calculation in bench
  qemu-img: prevent stack overflow in bench by using bottom half
  qemu-img: improve queue depth validation in img_bench

 qemu-img.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

-- 
2.42.2

[PATCH 1/4] qemu-img: fix division by zero in bench_cb() for zero-sized

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

This error was discovered by fuzzing qemu-img.

This commit fixes a division by zero error in the bench_cb() function
that occurs when using the bench command with a zero-sized image.

The issue arises because b->image_size can be zero, leading to a
division by zero in the modulo operation (b->offset %= b->image_size).
This patch adds a check for b->image_size == 0 and resets b->offset
to 0 in such cases, preventing the error.

Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
---
 qemu-img.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 89c93c1eb5..2044c22a4c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4488,7 +4488,11 @@ static void bench_cb(void *opaque, int ret)
          */
         b->in_flight++;
         b->offset += b->step;
-        b->offset %= b->image_size;
+        if (b->image_size == 0) {
+            b->offset = 0;
+        } else {
+            b->offset %= b->image_size;
+        }
         if (b->write) {
             acb = blk_aio_pwritev(b->blk, offset, b->qiov, 0, bench_cb, b);
         } else {
-- 
2.42.2

[PATCH 2/4] qemu-img: fix offset calculation in bench

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

This error was discovered by fuzzing qemu-img.

The current offset calculation leads to an EIO error
in block/block-backend.c: blk_check_byte_request():

 if (offset > len || len - offset < bytes) {
     return -EIO;
 }

This triggers the error message:
"qemu-img: Failed request: Input/output error".

Example of the issue:
 offset: 260076
 len: 260096
 bytes: 4096

This fix ensures that offset remains within a valid range.

Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 2044c22a4c..71c9fe496f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4491,7 +4491,7 @@ static void bench_cb(void *opaque, int ret)
         if (b->image_size == 0) {
             b->offset = 0;
         } else {
-            b->offset %= b->image_size;
+            b->offset %= b->image_size - b->bufsize;
         }
         if (b->write) {
             acb = blk_aio_pwritev(b->blk, offset, b->qiov, 0, bench_cb, b);
-- 
2.42.2

Re: [PATCH 2/4] qemu-img: fix offset calculation in bench

[Kevin Wolf]

Am 27.03.2025 um 17:24 hat gerben@altlinux.org geschrieben:
> From: Denis Rastyogin <gerben@altlinux.org>
> 
> This error was discovered by fuzzing qemu-img.
> 
> The current offset calculation leads to an EIO error
> in block/block-backend.c: blk_check_byte_request():
> 
>  if (offset > len || len - offset < bytes) {
>      return -EIO;
>  }
> 
> This triggers the error message:
> "qemu-img: Failed request: Input/output error".
> 
> Example of the issue:
>  offset: 260076
>  len: 260096
>  bytes: 4096
> 
> This fix ensures that offset remains within a valid range.
> 
> Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
> ---
>  qemu-img.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 2044c22a4c..71c9fe496f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4491,7 +4491,7 @@ static void bench_cb(void *opaque, int ret)
>          if (b->image_size == 0) {
>              b->offset = 0;
>          } else {
> -            b->offset %= b->image_size;
> +            b->offset %= b->image_size - b->bufsize;

The approach makes sense in principle, but you just introduced a new
division by zero here if image_size == bufsize (in this case we want to
use 0 as the new offset).

We probably also don't want to allow this to become negative.

Kevin

[PATCH 3/4] qemu-img: prevent stack overflow in bench by using bottom half

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

This error was discovered by fuzzing qemu-img.

Previously, new I/O requests were launched synchronously inside the
completion callback `bench_cb`, leading to deep recursion and stack
overflow. This patch moves the launching of new requests to a separate
function `bench_bh`, scheduled via `qemu_bh_schedule` to run in the event
loop context, thus unwinding the stack and preventing overflow.

Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
---
 qemu-img.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 71c9fe496f..5cbf3d18d7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4426,6 +4426,7 @@ typedef struct BenchData {
     int in_flight;
     bool in_flush;
     uint64_t offset;
+    QEMUBH *bh;
 } BenchData;
 
 static void bench_undrained_flush_cb(void *opaque, int ret)
@@ -4479,7 +4480,16 @@ static void bench_cb(void *opaque, int ret)
             }
         }
     }
+    if (b->n > b->in_flight && b->in_flight < b->nrreq) {
+        qemu_bh_schedule(b->bh);
+    }
+}
 
+static void bench_bh(void *opaque)
+{
+    BenchData *b = opaque;
+    BlockAIOCB *acb;
+    
     while (b->n > b->in_flight && b->in_flight < b->nrreq) {
         int64_t offset = b->offset;
         /* blk_aio_* might look for completed I/Os and kick bench_cb
@@ -4737,6 +4747,7 @@ static int img_bench(int argc, char **argv)
     }
 
     gettimeofday(&t1, NULL);
+    data.bh = qemu_bh_new(bench_bh, &data);
     bench_cb(&data, 0);
 
     while (data.n > 0) {
@@ -4755,6 +4766,9 @@ out:
     qemu_vfree(data.buf);
     blk_unref(blk);
 
+    if (data.bh) {
+        qemu_bh_delete(data.bh);
+    }
     if (ret) {
         return 1;
     }
-- 
2.42.2

Re: [PATCH 3/4] qemu-img: prevent stack overflow in bench by using bottom half

[Kevin Wolf]

Am 27.03.2025 um 17:24 hat gerben@altlinux.org geschrieben:
> From: Denis Rastyogin <gerben@altlinux.org>
> 
> This error was discovered by fuzzing qemu-img.
> 
> Previously, new I/O requests were launched synchronously inside the
> completion callback `bench_cb`, leading to deep recursion and stack
> overflow. This patch moves the launching of new requests to a separate
> function `bench_bh`, scheduled via `qemu_bh_schedule` to run in the event
> loop context, thus unwinding the stack and preventing overflow.
> 
> Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
> Signed-off-by: Denis Rastyogin <gerben@altlinux.org>

Normally, the callback shouldn't immediately be called, except for
errors, which take a different code path anyway.

Was this tested with the null block driver or with a backend that is
actually relevant in practice?

I wonder if switching qemu-img bench to coroutines would make sense. But
since this is a benchmarking tool, we need to measure the performance
difference from both an additional BH and from switching to coroutines
compared to the current state. In case of doubt, I'd leave this unfixed,
this isn't something that is run in production.

Kevin

[PATCH 4/4] qemu-img: improve queue depth validation in img_bench

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

This error was discovered by fuzzing qemu-img.

Currently, running `qemu-img bench -d 0` in img_bench is allowed,
which is a pointless operation and causes qemu-img to hang.

Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 5cbf3d18d7..4817bd9b05 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4581,7 +4581,7 @@ static int img_bench(int argc, char **argv)
         {
             unsigned long res;
 
-            if (qemu_strtoul(optarg, NULL, 0, &res) < 0 || res > INT_MAX) {
+            if (qemu_strtoul(optarg, NULL, 0, &res) <= 0 || res > INT_MAX) {
                 error_report("Invalid queue depth specified");
                 return 1;
             }
-- 
2.42.2

Re: [PATCH 4/4] qemu-img: improve queue depth validation in img_bench

[Kevin Wolf]

Am 27.03.2025 um 17:24 hat gerben@altlinux.org geschrieben:
> From: Denis Rastyogin <gerben@altlinux.org>
> 
> This error was discovered by fuzzing qemu-img.
> 
> Currently, running `qemu-img bench -d 0` in img_bench is allowed,
> which is a pointless operation and causes qemu-img to hang.
> 
> Signed-off-by: Denis Rastyogin <gerben@altlinux.org>

This one seems obvious, so I'm already taking it.

Thanks, applied to the block branch.

Kevin