Re: [Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies

Now that we allow clients to bypass buffer pre-initialization, it becomes more important to detect when a buggy server using structured replies does not send us enough bytes to cover the requested read size. Our check is not perfect (a server that duplicates reply chunks for byte 0 and omits byte 1 gets past our check), but this is a tighter sanity check so that we are less likely to report a successful read containing uninitialized memory on a buggy server.

Because we have a maximum read buffer size of 64M, and first check that the server's chunk fits bounds, we don't have to worry about overflowing a uint32_t, even if a server sends enough duplicate responses that an actual sum would overflow. --- lib/internal.h | 2 +- generator/states-reply-simple.c | 4 ++-- generator/states-reply-structured.c | 6 ++++-- lib/aio.c | 7 +++++-- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 885cee1..4121a5c 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -352,8 +352,8 @@ struct command { void *data; /* Buffer for read/write */ struct command_cb cb; enum state state; /* State to resume with on next POLLIN */ - bool data_seen; /* For read, true if at least one data chunk seen */ bool initialized; /* For read, true if getting a hole may skip memset */ + uint32_t data_seen; /* For read, cumulative size of data chunks seen */ uint32_t error; /* Local errno value */ }; diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 7dc26fd..2a7b9a9 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -40,7 +40,7 @@ STATE_MACHINE { if (cmd->error == 0 && cmd->type == NBD_CMD_READ) { h->rbuf = cmd->data; h->rlen = cmd->count; - cmd->data_seen = true; + cmd->data_seen = cmd->count; SET_NEXT_STATE (%RECV_READ_PAYLOAD); } else { diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 12c24f5..cabd543 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -354,7 +354,6 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */ assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true; /* Length of the data following. */ length -= 8; @@ -364,6 +363,8 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;

/* Now this is the byte offset in the read buffer. */ offset -= cmd->offset; @@ -422,13 +423,14 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */ assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true; /* Is the data within bounds? */ if (! structured_reply_in_bounds (offset, length, cmd)) { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;

/* Now this is the byte offset in the read buffer. */ offset -= cmd->offset; diff --git a/lib/aio.c b/lib/aio.c index 9744840..dc01f90 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -91,8 +91,11 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, assert (cmd->type != NBD_CMD_DISC); /* The spec states that a 0-length read request is unspecified; but * it is easy enough to treat it as successful as an extension. + * Conversely, make sure a server sending structured replies sent + * enough data chunks to cover the overall count (although we do not + * detect if it duplicated some bytes while omitting others). */ - if (type == NBD_CMD_READ && !cmd->data_seen && cmd->count && !error) + if (type == NBD_CMD_READ && cmd->data_seen != cmd->count && !error) error = EIO; /* Retire it from the list and free it. */ -- 2.36.1 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs