When reading to / writing from non-growable exports, we cap the I/O size by `offset - blk_len`. This will underflow for accesses that are completely past the disk end.
Check and handle that case explicitly. This is also enough to ensure that `offset + size` will not overflow; blk_len is int64_t, offset is uint32_t, `offset < blk_len`, so from `INT64_MAX + UINT32_MAX < UINT64_MAX` it follows that `offset + size` cannot overflow. Just one catch: We have to allow write accesses to growable exports past the EOF, so then we cannot rely on `offset < blk_len`, but have to verify explicitly that `offset + size` does not overflow. The negative consequences of not having this commit are luckily limited because blk_pread() and blk_pwrite() will reject post-EOF requests anyway, so a `size` underflow post-EOF will just result in an I/O error. So: - Post-EOF reads will incorrectly result in I/O errors instead of just 0-length reads. We will also attempt to allocate a very large buffer, which is wrong and not good, but not terrible. - Post-EOF writes on non-growable exports will result in I/O errors instead of 0-length writes (which generally indicate ENOSPC). - Post-EOF writes on growable exports can theoretically overflow on EOF and truncate the export down to a much too small size, but in practice, FUSE will never send an offset greater than signed INT_MAX, preventing a uint64_t overflow. (fuse_write_args_fill() in the kernel uses loff_t for the offset, which is signed.) Signed-off-by: Hanna Czenczek <[email protected]> --- block/export/fuse.c | 20 +++++++++++++++++++- tests/qemu-iotests/308 | 35 ++++++++++++++++++++++++++++++----- tests/qemu-iotests/308.out | 10 ++++++++++ 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/block/export/fuse.c b/block/export/fuse.c index d45c6b814f..af0a8de17b 100644 --- a/block/export/fuse.c +++ b/block/export/fuse.c @@ -657,6 +657,16 @@ static void fuse_read(fuse_req_t req, fuse_ino_t inode, return; } + if (offset >= blk_len) { + /* + * Technically libfuse does not allow returning a zero error code for + * read requests, but in practice this is a 0-length read (and a future + * commit will change this code anyway) + */ + fuse_reply_err(req, 0); + return; + } + if (offset + size > blk_len) { size = blk_len - offset; } @@ -717,7 +727,15 @@ static void fuse_write(fuse_req_t req, fuse_ino_t inode, const char *buf, return; } - if (offset + size > blk_len) { + if (offset >= blk_len && !exp->growable) { + fuse_reply_write(req, 0); + return; + } + + if (offset + size < offset) { + fuse_reply_err(req, EINVAL); + return; + } else if (offset + size > blk_len) { if (exp->growable) { ret = fuse_do_truncate(exp, offset + size, true, PREALLOC_MODE_OFF); if (ret < 0) { diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308 index 6ecb275555..a83c6fc01f 100755 --- a/tests/qemu-iotests/308 +++ b/tests/qemu-iotests/308 @@ -300,16 +300,34 @@ dd if=/dev/zero of="$EXT_MP" bs=1 count=64k seek=$orig_len \ conv=notrunc 2>&1 \ | _filter_testdir | _filter_imgfmt +# And one really squarely post-EOF write +dd if=/dev/zero of="$EXT_MP" bs=1 count=1 seek=$((orig_len + 32 * 1024)) \ + conv=notrunc 2>&1 \ + | _filter_testdir | _filter_imgfmt + +# Half-post-EOF reads +dd if="$EXT_MP" of=/dev/null bs=1 count=64k skip=$((orig_len - 32 * 1024)) \ + 2>&1 | _filter_testdir | _filter_imgfmt + +# And one really squarely post-EOF read +dd if="$EXT_MP" of=/dev/null bs=1 count=1 skip=$((orig_len + 32 * 1024)) \ + 2>&1 | _filter_testdir | _filter_imgfmt + echo echo '--- Resize export ---' # But we can truncate it explicitly; even with fallocate -fallocate -o "$orig_len" -l 64k "$EXT_MP" +# (Make sure we extend it to a length not divisible by 128k, we need that below) +bs=$((128 * 1024)) +extend_to=$(((orig_len + bs - 1) / bs * bs + bs / 2)) +extend_by=$((extend_to - orig_len)) + +fallocate -o "$orig_len" -l $extend_by "$EXT_MP" new_len=$(get_proto_len "$EXT_MP" "$TEST_IMG") -if [ "$new_len" != "$((orig_len + 65536))" ]; then +if [ "$new_len" != "$extend_to" ]; then echo 'ERROR: Unexpected post-truncate image size:' - echo "$new_len != $((orig_len + 65536))" + echo "$new_len != $extend_to" else echo 'OK: Post-truncate image size is as expected' fi @@ -322,6 +340,13 @@ else echo "$orig_disk_usage => $new_disk_usage" fi +# Use this opportunity to test a read access across the (now no longer so much +# aligned) EOF. dd can only do requests with a length of its block size, and +# all of its seek/skip values are in bs units, so it is hard to do a request +# across the EOF if the EOF is at a power of two (64M). +dd if="$EXT_MP" of=/dev/null bs=$bs count=2 skip=$((extend_to / bs)) \ + 2>&1 | _filter_testdir | _filter_imgfmt + echo echo '--- Try growing growable export ---' @@ -338,9 +363,9 @@ dd if=/dev/zero of="$EXT_MP" bs=1 count=64k seek=$new_len conv=notrunc 2>&1 \ | _filter_testdir | _filter_imgfmt new_len=$(get_proto_len "$EXT_MP" "$TEST_IMG") -if [ "$new_len" != "$((orig_len + 131072))" ]; then +if [ "$new_len" != "$((extend_to + 65536))" ]; then echo 'ERROR: Unexpected post-grow image size:' - echo "$new_len != $((orig_len + 131072))" + echo "$new_len != $((extend_to + 65536))" else echo 'OK: Post-grow image size is as expected' fi diff --git a/tests/qemu-iotests/308.out b/tests/qemu-iotests/308.out index 2d7a38d63d..ebeaf64b48 100644 --- a/tests/qemu-iotests/308.out +++ b/tests/qemu-iotests/308.out @@ -134,11 +134,21 @@ wrote 65536/65536 bytes at offset 1048576 dd: error writing 'TEST_DIR/t.IMGFMT.fuse': No space left on device 1+0 records in 0+0 records out +dd: error writing 'TEST_DIR/t.IMGFMT.fuse': No space left on device +1+0 records in +0+0 records out +32768+0 records in +32768+0 records out +dd: TEST_DIR/t.IMGFMT.fuse: cannot skip to specified offset +0+0 records in +0+0 records out --- Resize export --- (OK: Lengths of export and original are the same) OK: Post-truncate image size is as expected OK: Disk usage grew with fallocate +0+1 records in +0+1 records out --- Try growing growable export --- {'execute': 'block-export-del', -- 2.53.0
