[Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
Nir Soffer
nsoffer at redhat.com
Tue May 31 15:59:25 UTC 2022
On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake at redhat.com> wrote:
>
> 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.
Nice!
> 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;
This does not feel right. if you received more data, it should be counted,
and if this causes data_seen to be bigger than cmd->count, isn't this a fatal
error?
> /* 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;
Same here
> /* 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 at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
Nir
More information about the Libguestfs
mailing list