qemu-io: check the size of the I/O requests

Passing a request size larger than INT_MAX to any of the I/O commands results in an error. While 'read' and 'write' handle the error correctly, 'aio_read' and 'aio_write' hit an assertion:

blk_aio_read_entry: Assertion `rwco->qiov->size == acb->bytes' failed.

The reason is that the QEMU I/O code cannot handle request sizes larger than INT_MAX, so this patch makes qemu-io check that all values are within range.

Signed-off-by Alberto Garcia <[email protected]>

---
 qemu-io-cmds.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Ideally, it would be nice to fix the block layer to allow larger requests (since we already have code to auto-fragment down to device limits, we should be able to rely on that code instead of having to duplicate artificial constraints everywhere else in the tree). But that's a bigger task, and this is a good patch in the interim.

Quoted 5 lines · Eric Blake

Ideally, it would be nice to fix the block layer to allow larger requests (since we already have code to auto-fragment down to device limits, we should be able to rely on that code instead of having to duplicate artificial constraints everywhere else in the tree). But that's a bigger task, and this is a good patch in the interim.

Related question: what's the largest request than a guest can theoretically submit?

Quoted 8 lines

Quoted 5 lines

Ideally, it would be nice to fix the block layer to allow larger requests (since we already have code to auto-fragment down to device limits, we should be able to rely on that code instead of having to duplicate artificial constraints everywhere else in the tree). But that's a bigger task, and this is a good patch in the interim.

Related question: what's the largest request than a guest can theoretically submit?

off_t supports up to 2^63 (not 2^64, because it is a signed type). Ideally, we should be constrained only by the disk size (as no one actually has 2^63 bytes of storage available), by using uint64_t offset AND length in all our APIs; but right now, we still have a lot of 32-bit length issues, and often signed length limiting us to 2^31 depending on the API.

Quoted 9 lines · Eric Blake

Quoted 8 lines

Quoted 5 lines

Ideally, it would be nice to fix the block layer to allow larger requests (since we already have code to auto-fragment down to device limits, we should be able to rely on that code instead of having to duplicate artificial constraints everywhere else in the tree). But that's a bigger task, and this is a good patch in the interim.

Related question: what's the largest request than a guest can theoretically submit?

Quoted 6 lines

off_t supports up to 2^63 (not 2^64, because it is a signed type). Ideally, we should be constrained only by the disk size (as no one actually has 2^63 bytes of storage available), by using uint64_t offset AND length in all our APIs; but right now, we still have a lot of 32-bit length issues, and often signed length limiting us to 2^31 depending on the API.

My question was more like: what happens if the guest submits a request with the maximum possible size? Does QEMU handle that correctly?

Passing a request size larger than INT_MAX to any of the I/O commands results in an error. While 'read' and 'write' handle the error correctly, 'aio_read' and 'aio_write' hit an assertion:

blk_aio_read_entry: Assertion `rwco->qiov->size == acb->bytes' failed.

The reason is that the QEMU I/O code cannot handle request sizes larger than INT_MAX, so this patch makes qemu-io check that all values are within range.

Signed-off-by Alberto Garcia <[email protected]>

---
 qemu-io-cmds.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

qemu-io-cmds.c +0 -0

index 95bcde1d88..d806a83076 100644
@@ -388,9 +388,14 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov,
             goto fail;
         }

qemu-io-cmds.c +6 -6

+            printf("The total number of bytes exceed the maximum size %d\n",
+                   INT_MAX);
             goto fail;
         }
 

@@ -682,9 +687,8 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         print_cvtnum_err(count, argv[optind]);
         return 0;
-    } else if (count > SIZE_MAX) {
-        printf("length cannot exceed %" PRIu64 ", given %s\n",
-               (uint64_t) SIZE_MAX, argv[optind]);
+    } else if (count > INT_MAX) {
+        printf("length cannot exceed %d, given %s\n", INT_MAX, argv[optind]);
         return 0;
     }
 

@@ -1004,9 +1008,8 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         print_cvtnum_err(count, argv[optind]);
         return 0;
-    } else if (count > SIZE_MAX) {
-        printf("length cannot exceed %" PRIu64 ", given %s\n",
-               (uint64_t) SIZE_MAX, argv[optind]);
+    } else if (count > INT_MAX) {
+        printf("length cannot exceed %d, given %s\n", INT_MAX, argv[optind]);
         return 0;
     }
 

Quoted 1 line

+ if (count > INT_MAX - len) {

How about using BDRV_REQUEST_MAX_BYTES instead?

(not yet in master, just in my block branch)

Passing a request size larger than INT_MAX to any of the I/O commands results in an error. While 'read' and 'write' handle the error correctly, 'aio_read' and 'aio_write' hit an assertion:

blk_aio_read_entry: Assertion `rwco->qiov->size == acb->bytes' failed.

The reason is that the QEMU I/O code cannot handle request sizes larger than INT_MAX, so this patch makes qemu-io check that all values are within range.

Signed-off-by Alberto Garcia <[email protected]>

---
 qemu-io-cmds.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Thanks, applied to my block tree, with
%s/INT_MAX/BDRV_REQUEST_MAX_BYTES/g:

Quoted 2 lines · Max Reitz

Thanks, applied to my block tree, with
%s/INT_MAX/BDRV_REQUEST_MAX_BYTES/g:

I think you can use %d to print BDRV_REQUEST_MAX_BYTES, after all the definition guarantees that it won't be larger than MIN(SIZE_MAX,INT_MAX)

The type of qiov->size is size_t but the QEMU I/O code uses int all over the place to measure request sizes. Overflows are likely going to be detected by any of the many assert(qiov->size == nbytes), where nbytes is an int, but let's add proper checks in the QEMUIOVector initialization, which is where it belongs.

Signed-off-by Alberto Garcia <[email protected]>

---
 util/iov.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

The type of qiov->size is size_t but the QEMU I/O code uses int all over the place to measure request sizes. Overflows are likely going to be detected by any of the many assert(qiov->size == nbytes), where nbytes is an int, but let's add proper checks in the QEMUIOVector initialization, which is where it belongs.

Signed-off-by Alberto Garcia <[email protected]>

---
 util/iov.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

util/iov.c +6 -1

index 74e6ca8ed7..6b5cc9203c 100644
@@ -283,13 +283,18 @@ void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov)
     qiov->niov = niov;
     qiov->nalloc = -1;
     qiov->size = 0;
-    for (i = 0; i < niov; i++)
+    for (i = 0; i < niov; i++) {
+        assert(iov[i].iov_len <= INT_MAX);
+        assert(qiov->size <= INT_MAX - iov[i].iov_len);
         qiov->size += iov[i].iov_len;
+    }
 }
 
 void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len)
 {
     assert(qiov->nalloc != -1);
+    assert(len <= INT_MAX);
+    assert(qiov->size <= INT_MAX - len);
 
     if (qiov->niov == qiov->nalloc) {
         qiov->nalloc = 2 * qiov->nalloc + 1;

The assertion probably makes sense even for them, considering that size_t does not have a constant size. But I'm not entirely sold that the I/O vector creation is actually the place where the assertions belong.

Quoted 4 lines · Max Reitz

The assertion probably makes sense even for them, considering that size_t does not have a constant size. But I'm not entirely sold that the I/O vector creation is actually the place where the assertions belong.

I was actually just reading virtio_blk_handle_request() and I see that the I/O vector is initialized before the check for the maximum request size that returns VIRTIO_BLK_S_IOERR.

So you're probably right and it's safer not to add that assertion.

Quoted 5 lines

I was actually just reading virtio_blk_handle_request() and I see that the I/O vector is initialized before the check for the maximum request size that returns VIRTIO_BLK_S_IOERR.

So you're probably right and it's safer not to add that assertion.

So... Apply patch 1 and hope we do everything right in the future? :-)