Nir Soffer
2022-May-31 15:59 UTC
[Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
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/libguestfsNir
Eric Blake
2022-May-31 16:13 UTC
[Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
On Tue, May 31, 2022 at 06:59:25PM +0300, Nir Soffer wrote:> 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. > > ---> > +++ 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?cmd->count is at most 64M; it represents how much we asked the server to provide. length was just validated (in the elided statement between these two hunks) to be <= cmd->count (otherwise, the server is known-buggy for replying with more than we asked, and we've already moved to %.DEAD state). cmd->data_seen is a cumulative count of all bytes seen in prior chunks, plus the current chunk. If we have not yet passed cmd->count, then this chunk counts towards the cumulative limit (and there is no overflow, since 64M*2 < UINT32_MAX). If we have already passed cmd->count (in a previous chunk), then we don't further increase cmd->count, but we already know that we will fail the overall read later. In other words, we can stop incrementing cmd->data_seen as soon as we know it exceeds cmd->count, and by stopping early, we still detect server bugs without overflowing uint32_t. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org