Re: [Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
On Tue, May 31, 2022 at 10:32:23PM +0300, Nir Soffer wrote: > > > But we can have this case: > > > > > > 1. ask for 32m > > > 2. server sends 16m (data_seen increase to 16m) > > > 3. server sends 16m (data_seen increase to 32m) > > > 4. server sends 1m (data_seen does not increase) > > > > Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m. > > Right! I missed this. > > > Then, later on when retiring the command, we note that 33m != 32m and > > fail the read with EIO (if it has not already failed for other > > reasons). > > > > > 5. entire request succeeds > > > > > > Shouldn't we fail if server sends unexpected data? > > > > > > If we detected that all data was received, and we get > > > unexpected data, why not fail immediately? > > > > > > cmd->data_seen += length > > > if (cmd->data_seen > cmd->count) > > > switch to dead state? > > > > Switching immediately to a dead state is also possible, but it's nice > > to try and keep the connection alive as long as we can with a nice > > diagnosis of a failed CMD_READ but still allow further commands, > > rather than an abrupt disconnect that takes out all other use of the > > server. > > I agree, this is better. > > Reviewed-by: Nir Soffer I've pushed this one as edd8f5c; the rest of the series will be tweaked and posted as v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
On Tue, May 31, 2022 at 10:23 PM Eric Blake wrote:
>
> On Tue, May 31, 2022 at 08:40:57PM +0300, Nir Soffer wrote:
> > > > > @@ -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.
> >
> > But we can have this case:
> >
> > 1. ask for 32m
> > 2. server sends 16m (data_seen increase to 16m)
> > 3. server sends 16m (data_seen increase to 32m)
> > 4. server sends 1m (data_seen does not increase)
>
> Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m.
Right! I missed this.
> Then, later on when retiring the command, we note that 33m != 32m and
> fail the read with EIO (if it has not already failed for other
> reasons).
>
> > 5. entire request succeeds
> >
> > Shouldn't we fail if server sends unexpected data?
> >
> > If we detected that all data was received, and we get
> > unexpected data, why not fail immediately?
> >
> > cmd->data_seen += length
> > if (cmd->data_seen > cmd->count)
> > switch to dead state?
>
> Switching immediately to a dead state is also possible, but it's nice
> to try and keep the connection alive as long as we can with a nice
> diagnosis of a failed CMD_READ but still allow further commands,
> rather than an abrupt disconnect that takes out all other use of the
> server.
I agree, this is better.
Reviewed-by: Nir Soffer
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
On Tue, May 31, 2022 at 08:40:57PM +0300, Nir Soffer wrote:
> > > > @@ -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.
>
> But we can have this case:
>
> 1. ask for 32m
> 2. server sends 16m (data_seen increase to 16m)
> 3. server sends 16m (data_seen increase to 32m)
> 4. server sends 1m (data_seen does not increase)
Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m.
Then, later on when retiring the command, we note that 33m != 32m and
fail the read with EIO (if it has not already failed for other
reasons).
> 5. entire request succeeds
>
> Shouldn't we fail if server sends unexpected data?
>
> If we detected that all data was received, and we get
> unexpected data, why not fail immediately?
>
> cmd->data_seen += length
> if (cmd->data_seen > cmd->count)
> switch to dead state?
Switching immediately to a dead state is also possible, but it's nice
to try and keep the connection alive as long as we can with a nice
diagnosis of a failed CMD_READ but still allow further commands,
rather than an abrupt disconnect that takes out all other use of the
server.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
On Tue, May 31, 2022 at 7:13 PM Eric Blake wrote:
>
> On Tue, May 31, 2022 at 06:59:25PM +0300, Nir Soffer wrote:
> > On Tue, May 31, 2022 at 5:15 PM Eric Blake 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.
But we can have this case:
1. ask for 32m
2. server sends 16m (data_seen increase to 16m)
3. server sends 16m (data_seen increase to 32m)
4. server sends 1m (data_seen does not increase)
5. entire request succeeds
Shouldn't we fail if server sends unexpected data?
If we detected that all data was received, and we get
unexpected data, why not fail immediately?
cmd->data_seen += length
if (cmd->data_seen > cmd->count)
switch to dead state?
Nir
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [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 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies
On Tue, May 31, 2022 at 5:15 PM Eric Blake 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] [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@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
