When a server advertises an unaligned size but no block sizes, the
code was rounding up to a sector-aligned size (a known limitation of
bdrv_getlength(); maybe 4.1 will fix it to be byte-accurate), then
assuming a request_alignment of 512 (the recommendation of the NBD
spec for maximum portability). However, this means that qemu will
actually attempt to access the padding bytes of the trailing partial
sector, without realizing that it is risky.
An easy demonstration, using nbdkit as the server:
$ nbdkit -fv random size=1023
$ qemu-io -r -f raw -c 'r -v 0 1023' nbd://localhost:10809
read failed: Invalid argument
because the client rounded the request up to 1024 bytes, which nbdkit
then rejected as beyond the advertised size of 1023.
Note that qemu as the server refuses to send an unaligned size, as it
has already rounded the unaligned image up to sector size, and then
happily resizes the image on access (at least when serving a POSIX
file over NBD). But since third-party servers may decide to kill the
connection when we access out of bounds, it's better to just ignore
the unaligned tail than it is to risk problems. We can undo this hack
later once the block layer quits rounding sizes inappropriately.
Reported-by: Richard W.M. Jones <rjo...@redhat.com>
Signed-off-by: Eric Blake <ebl...@redhat.com>
---
This is the minimal patch to avoid our client behaving out-of-spec,
but I don't like that it makes the tail of the server's file
unreadable. A better solution of auditing the block layer to use a
proper byte length everywhere instead of rounding up may be 4.1
material, but is certainly not appropriate for 4.0-rc2. I could also
teach the NBD code to compare every request from the block length
against client.info.size, and manually handle the tail itself, but
that feels like a lot of pain (for something the block layer should be
able to do generically, and where any NBD-specific tail-handling code
just slows down the common case and would be ripped out again once the
block layer quits rounding up). I'm willing to include this with my
other NBD patches slated for -rc2 as part of my suite of improved
third-party interoperability, but only if we agree that the truncation
is appropriate.
Note that nbdkit includes '--filter=truncate round-up=512' as a way to
expose the unaligned tail without making qemu trigger out-of-bounds
operations: reads of the tail now succeed with contents of 0; writes
fail with ENOSPC unless the contents requested to be written into the
tail are all 0s. So not fixing the bug for 4.0, and telling people to
use nbdkit's truncate filter instead, is also a viable option.
block/nbd.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block/nbd.c b/block/nbd.c
index 2e72df528ac..a2cd365f646 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -463,7 +463,17 @@ static int64_t nbd_getlength(BlockDriverState *bs)
{
BDRVNBDState *s = bs->opaque;
- return s->client.info.size;
+ /*
+ * HACK: Round down to sector alignment. qemu as server always
+ * advertises a sector-aligned image, so we only ever see
+ * unaligned sizes with third-party servers. The block layer
+ * defaults to rounding up, but that can result in us attempting
+ * to read beyond EOF from the remote server; better is to
+ * forcefully round down here and just treat the last few bytes
+ * from the server as unreadable. This can all go away once the
+ * block layer uses actual byte lengths instead of rounding up.
+ */
+ return QEMU_ALIGN_DOWN(s->client.info.size, BDRV_SECTOR_SIZE);
}
static void nbd_detach_aio_context(BlockDriverState *bs)
--
2.20.1
