diff mbox series

[v7,01/12] nbd/server: Support a request payload

Message ID 20230925192229.3186470-15-eblake@redhat.com
State New
Headers show
Series NBD 64-bit extensions for qemu | expand

Commit Message

Eric Blake Sept. 25, 2023, 7:22 p.m. UTC
Upcoming additions to support NBD 64-bit effect lengths allow for the
possibility to distinguish between payload length (capped at 32M) and
effect length (64 bits, although we generally assume 63 bits because
of off_t limitations).  Without that extension, only the NBD_CMD_WRITE
request has a payload; but with the extension, it makes sense to allow
at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
in a future patch (where the payload is a limited-size struct that in
turn gives the real effect length as well as a subset of known ids for
which status is requested).  Other future NBD commands may also have a
request payload, so the 64-bit extension introduces a new
NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
length is a payload length or an effect length, rather than
hard-coding the decision based on the command.

According to the spec, a client should never send a command with a
payload without the negotiation phase proving such extension is
available.  So in the unlikely event the bit is set or cleared
incorrectly, the client is already at fault; if the client then
provides the payload, we can gracefully consume it off the wire and
fail the command with NBD_EINVAL (subsequent checks for magic numbers
ensure we are still in sync), while if the client fails to send
payload we block waiting for it (basically deadlocking our connection
to the bad client, but not negatively impacting our ability to service
other clients, so not a security risk).  Note that we do not support
the payload version of BLOCK_STATUS yet.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

v7: another try at better logic [Vladimir]

v5: retitled from v4 13/24, rewrite on top of previous patch's switch
statement [Vladimir]

v4: less indentation on several 'if's [Vladimir]
---
 nbd/server.c     | 37 +++++++++++++++++++++++++++++++++----
 nbd/trace-events |  1 +
 2 files changed, 34 insertions(+), 4 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Sept. 27, 2023, 8:55 a.m. UTC | #1
On 25.09.23 22:22, Eric Blake wrote:
> Upcoming additions to support NBD 64-bit effect lengths allow for the
> possibility to distinguish between payload length (capped at 32M) and
> effect length (64 bits, although we generally assume 63 bits because
> of off_t limitations).  Without that extension, only the NBD_CMD_WRITE
> request has a payload; but with the extension, it makes sense to allow
> at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
> in a future patch (where the payload is a limited-size struct that in
> turn gives the real effect length as well as a subset of known ids for
> which status is requested).  Other future NBD commands may also have a
> request payload, so the 64-bit extension introduces a new
> NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
> length is a payload length or an effect length, rather than
> hard-coding the decision based on the command.
> 
> According to the spec, a client should never send a command with a
> payload without the negotiation phase proving such extension is
> available.  So in the unlikely event the bit is set or cleared
> incorrectly, the client is already at fault; if the client then
> provides the payload, we can gracefully consume it off the wire and
> fail the command with NBD_EINVAL (subsequent checks for magic numbers
> ensure we are still in sync), while if the client fails to send
> payload we block waiting for it (basically deadlocking our connection
> to the bad client, but not negatively impacting our ability to service
> other clients, so not a security risk).  Note that we do not support
> the payload version of BLOCK_STATUS yet.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> v7: another try at better logic [Vladimir]
> 
> v5: retitled from v4 13/24, rewrite on top of previous patch's switch
> statement [Vladimir]
> 
> v4: less indentation on several 'if's [Vladimir]
> ---
>   nbd/server.c     | 37 +++++++++++++++++++++++++++++++++----
>   nbd/trace-events |  1 +
>   2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 7a6f95071f8..1eabcfc908d 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2322,9 +2322,11 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>                                                  Error **errp)
>   {
>       NBDClient *client = req->client;
> +    bool extended_with_payload;
>       bool check_length = false;
>       bool check_rofs = false;
>       bool allocate_buffer = false;
> +    bool payload_okay = false;
>       unsigned payload_len = 0;
>       int valid_flags = NBD_CMD_FLAG_FUA;
>       int ret;
> @@ -2338,6 +2340,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> 
>       trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
>                                                nbd_cmd_lookup(request->type));
> +    extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
> +        request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
> +    if (extended_with_payload) {
> +        payload_len = request->len;
> +        check_length = true;
> +    }
> +
>       switch (request->type) {
>       case NBD_CMD_DISC:
>           /* Special case: we're going to disconnect without a reply,
> @@ -2354,6 +2363,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>           break;
> 
>       case NBD_CMD_WRITE:
> +        if (client->mode >= NBD_MODE_EXTENDED) {
> +            if (!extended_with_payload) {
> +                /* The client is noncompliant. Trace it, but proceed. */
> +                trace_nbd_co_receive_ext_payload_compliance(request->from,
> +                                                            request->len);
> +            }
> +            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> +        }
> +        payload_okay = true;
>           payload_len = request->len;
>           check_length = true;
>           allocate_buffer = true;
> @@ -2395,6 +2413,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>                      request->len, NBD_MAX_BUFFER_SIZE);
>           return -EINVAL;
>       }
> +    if (payload_len && !payload_okay) {
> +        /*
> +         * For now, we don't support payloads on other commands; but
> +         * we can keep the connection alive by ignoring the payload.
> +         */
> +        assert(request->type != NBD_CMD_WRITE);
> +        request->len = 0;

So, actually we handle a syntactic request with len=0 and return success... I'm afraid, that in the most scenarios that would not be what client want, but client will be confused by success return.

So, for example, if client pass READ with positive length and accidentlly set NBD_CMD_FLAG_PAYLOAD_LEN bit, it will get successful result with wrong length=0.
Or, for WRITE_ZEROES (with accidental NBD_CMD_FLAG_PAYLOAD_LEN) it will just get successful result, when actually nothing is done.

> +    }
>       if (allocate_buffer) {
>           /* READ, WRITE */
>           req->data = blk_try_blockalign(client->exp->common.blk,
> @@ -2405,10 +2431,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>           }
>       }
>       if (payload_len) {
> -        /* WRITE */
> -        assert(req->data);
> -        ret = nbd_read(client->ioc, req->data, payload_len,
> -                       "CMD_WRITE data", errp);
> +        if (payload_okay) {
> +            assert(req->data);
> +            ret = nbd_read(client->ioc, req->data, payload_len,
> +                           "CMD_WRITE data", errp);
> +        } else {
> +            ret = nbd_drop(client->ioc, payload_len, errp);
> +        }
>           if (ret < 0) {
>               return -EIO;
>           }
> diff --git a/nbd/trace-events b/nbd/trace-events
> index f9dccfcfb44..c1a3227613f 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t
>   nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'"
>   nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)"
>   nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received: cookie = %" PRIu64 ", len = %" PRIu64
> +nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64
>   nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32
>   nbd_trip(void) "Reading request"
>
Eric Blake Sept. 27, 2023, 3:59 p.m. UTC | #2
On Wed, Sep 27, 2023 at 11:55:41AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 25.09.23 22:22, Eric Blake wrote:
> > Upcoming additions to support NBD 64-bit effect lengths allow for the
> > possibility to distinguish between payload length (capped at 32M) and
> > effect length (64 bits, although we generally assume 63 bits because
> > of off_t limitations).  Without that extension, only the NBD_CMD_WRITE
> > request has a payload; but with the extension, it makes sense to allow
> > at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
> > in a future patch (where the payload is a limited-size struct that in
> > turn gives the real effect length as well as a subset of known ids for
> > which status is requested).  Other future NBD commands may also have a
> > request payload, so the 64-bit extension introduces a new
> > NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
> > length is a payload length or an effect length, rather than
> > hard-coding the decision based on the command.
> > 
> > According to the spec, a client should never send a command with a
> > payload without the negotiation phase proving such extension is
> > available.  So in the unlikely event the bit is set or cleared
> > incorrectly, the client is already at fault; if the client then
> > provides the payload, we can gracefully consume it off the wire and
> > fail the command with NBD_EINVAL (subsequent checks for magic numbers
> > ensure we are still in sync), while if the client fails to send
> > payload we block waiting for it (basically deadlocking our connection
> > to the bad client, but not negatively impacting our ability to service
> > other clients, so not a security risk).  Note that we do not support
> > the payload version of BLOCK_STATUS yet.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > 
> > v7: another try at better logic [Vladimir]
> > 
> > v5: retitled from v4 13/24, rewrite on top of previous patch's switch
> > statement [Vladimir]
> > 
> > v4: less indentation on several 'if's [Vladimir]
> > ---
> >   nbd/server.c     | 37 +++++++++++++++++++++++++++++++++----
> >   nbd/trace-events |  1 +
> >   2 files changed, 34 insertions(+), 4 deletions(-)
> > 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 7a6f95071f8..1eabcfc908d 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -2322,9 +2322,11 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> >                                                  Error **errp)
> >   {
> >       NBDClient *client = req->client;
> > +    bool extended_with_payload;
> >       bool check_length = false;
> >       bool check_rofs = false;
> >       bool allocate_buffer = false;
> > +    bool payload_okay = false;
> >       unsigned payload_len = 0;
> >       int valid_flags = NBD_CMD_FLAG_FUA;
> >       int ret;
> > @@ -2338,6 +2340,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> > 
> >       trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
> >                                                nbd_cmd_lookup(request->type));
> > +    extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
> > +        request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
> > +    if (extended_with_payload) {
> > +        payload_len = request->len;
> > +        check_length = true;
> > +    }
> > +
> >       switch (request->type) {
> >       case NBD_CMD_DISC:
> >           /* Special case: we're going to disconnect without a reply,
> > @@ -2354,6 +2363,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> >           break;
> > 
> >       case NBD_CMD_WRITE:
> > +        if (client->mode >= NBD_MODE_EXTENDED) {
> > +            if (!extended_with_payload) {
> > +                /* The client is noncompliant. Trace it, but proceed. */
> > +                trace_nbd_co_receive_ext_payload_compliance(request->from,
> > +                                                            request->len);
> > +            }
> > +            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> > +        }
> > +        payload_okay = true;
> >           payload_len = request->len;
> >           check_length = true;
> >           allocate_buffer = true;
> > @@ -2395,6 +2413,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> >                      request->len, NBD_MAX_BUFFER_SIZE);
> >           return -EINVAL;
> >       }
> > +    if (payload_len && !payload_okay) {
> > +        /*
> > +         * For now, we don't support payloads on other commands; but
> > +         * we can keep the connection alive by ignoring the payload.
> > +         */
> > +        assert(request->type != NBD_CMD_WRITE);
> > +        request->len = 0;
> 
> So, actually we handle a syntactic request with len=0 and return success... I'm afraid, that in the most scenarios that would not be what client want, but client will be confused by success return.

If request->len == 0, then payload_len is also 0 and we do not enter
this block, even when !payload_okay.  A 0-byte NBD_CMD_READ is already
unspecified by the NBD spec (although I think we do permit it as a
no-op where we return success).  But that is not the whole picture -
even if the client passes a payload length 0f 0, it is important to
remember that NBD_CMD_READ did not add NBD_CMD_FLAG_PAYLOAD_LEN to
valid_flags, so later in this function, whether or not we have payload
bytes to slurp off, we will still fail the command with EINVAL for
passing an unexpected flag when doing the comparison against
valid_flags.

But I agree that adding a comment to that effect would help.

> 
> So, for example, if client pass READ with positive length and accidentlly set NBD_CMD_FLAG_PAYLOAD_LEN bit, it will get successful result with wrong length=0.

In that particular case, you argue the client did NOT pass in payload
bytes; so our attempt to read payload bytes will block until the
client issues enough subsequent commands to actually satisfy the
payload_len (or we see early EOF when the client hangs up after
noticing we have not responded to commands in a while).  We are
effectively ignoring all such subsequent commands (having treated them
as NBD_CMD_READ payload instead of independent commands), and if we
finally do get enough bytes to satisfy the full payload, it is highly
likely that we will not land on a clean command boundary and
disconnect when we see a magic number mismatch.  Less likely is the
payload_len ending exactly on a command boundary; but hopefully our
return of NBD_EINVAL will explain to the client why we have ignored
its subsequent commands.  At any rate, the bug is in the client for
sending an unsupported flag; anything we do beyond that point is best
effort, and the most important part is that if we attempted to keep
the connection alive, we are at least not letting the client cause us
to do unintended actions.

> Or, for WRITE_ZEROES (with accidental NBD_CMD_FLAG_PAYLOAD_LEN) it will just get successful result, when actually nothing is done.

Again, once we consume payload_len bytes (if we did not outright close
the connection because the count was larger than 32M and we were
unwilling to wade through that many payload bytes, thanks to
check_length), we are most likely to hit a magic number mismatch after
having ignored all intermediate commands that came in; but the
NBD_COMMAND_WRITE_ZEROES will receive an NBD_EINVAL failure, not
success, for not satisfying valid_flags.

Which do we think is more likely: a client accidentally setting the
flag but not passing payload, or a client purposefully setting the
flag and including payload?  If the former, then it may be nicer to
just disconnect any time we see the flag set unexpectedly; trying to
keep the connection alive will ignore subsequent client commands, and
likely (but not always) eventually result in a magic number mismatch
once payload_len bytes are ignored.  If the latter, then skipping
payload bytes keeps us in sync, so that we can read the next command.

> 
> > +    }
> >       if (allocate_buffer) {
> >           /* READ, WRITE */
> >           req->data = blk_try_blockalign(client->exp->common.blk,
> > @@ -2405,10 +2431,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> >           }
> >       }
> >       if (payload_len) {
> > -        /* WRITE */
> > -        assert(req->data);
> > -        ret = nbd_read(client->ioc, req->data, payload_len,
> > -                       "CMD_WRITE data", errp);
> > +        if (payload_okay) {
> > +            assert(req->data);
> > +            ret = nbd_read(client->ioc, req->data, payload_len,
> > +                           "CMD_WRITE data", errp);
> > +        } else {
> > +            ret = nbd_drop(client->ioc, payload_len, errp);

We could also try to be a bit more complicated by peeking at the next
few bytes: if they look like a magic number of the next request,
assume the client set the bit accidentally but didn't send a payload
after all; for anything else, assume the client did pass a payload.
But adding in machinery to peek at a prefix is more complex than
either assuming a payload is always present (as done in this patch) or
assuming the bit was in error (and dropping the connection
unconditionally).  Preferences?

At any rate, deciding what to do when dealing with a non-compliant
client should NOT hold up reviews of the rest of the series, which
only worry about compliant clients that never trigger this issue.
Worded differently, the important aspect of the review of this patch
is whether we see any scenario where a non-compliant client could
cause the server to misbehave, at which point declaring that we would
prefer to disconnect immediately rather than attempt to resync may be
safer, even though it is more abrupt.  But if we are satisfied that
the ill-behaved client cannot crash the server, trying to keep the
connection alive (at least, until we later hit a magic number
mismatch) is probably nicer.

> > +        }
> >           if (ret < 0) {
> >               return -EIO;
> >           }
Vladimir Sementsov-Ogievskiy Sept. 28, 2023, 9:09 a.m. UTC | #3
On 27.09.23 18:59, Eric Blake wrote:
> We could also try to be a bit more complicated by peeking at the next
> few bytes: if they look like a magic number of the next request,
> assume the client set the bit accidentally but didn't send a payload
> after all; for anything else, assume the client did pass a payload.
> But adding in machinery to peek at a prefix is more complex than
> either assuming a payload is always present (as done in this patch) or
> assuming the bit was in error (and dropping the connection
> unconditionally).  Preferences?


Ohh, you are right, thanks for comprehensive explanation. I really missed some things you are saying about. Yes, now I agree that "payload always exist when flag is set" is the best effort. Finally, that was our aim of the protocol design: make it more context independent. Probably, we may fix that in specification as preferable or at least possible server behavior about non-compliant client.

r-b coming soon, I just need to take another look with corrected picture in mind.
Eric Blake Sept. 28, 2023, 2:33 p.m. UTC | #4
On Thu, Sep 28, 2023 at 12:09:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 27.09.23 18:59, Eric Blake wrote:
> > We could also try to be a bit more complicated by peeking at the next
> > few bytes: if they look like a magic number of the next request,
> > assume the client set the bit accidentally but didn't send a payload
> > after all; for anything else, assume the client did pass a payload.
> > But adding in machinery to peek at a prefix is more complex than
> > either assuming a payload is always present (as done in this patch) or
> > assuming the bit was in error (and dropping the connection
> > unconditionally).  Preferences?
> 
> 
> Ohh, you are right, thanks for comprehensive explanation. I really missed some things you are saying about. Yes, now I agree that "payload always exist when flag is set" is the best effort. Finally, that was our aim of the protocol design: make it more context independent. Probably, we may fix that in specification as preferable or at least possible server behavior about non-compliant client.

One other possibility I just thought of: have a heuristic where the
flag set with h->request_length less than 512 bytes is likely to
indicate an intentional payload (even if for a command where we
weren't expecting payload, so still a client error); while the flag
set wtih h->request_length >= 512 bytes is likely to be a mistaken
setting of the flag (but also still a client error).  NBD_CMD_WRITE is
probably the only command that will ever need to send a payload larger
than one sector, but that command already has handling to accept
payloads of all sizes because we know what to do with them and where
the client is not in error.

> 
> r-b coming soon, I just need to take another look with corrected picture in mind.
> 
> -- 
> Best regards,
> Vladimir
>
Vladimir Sementsov-Ogievskiy Sept. 28, 2023, 8:52 p.m. UTC | #5
On 28.09.23 17:33, Eric Blake wrote:
> On Thu, Sep 28, 2023 at 12:09:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 27.09.23 18:59, Eric Blake wrote:
>>> We could also try to be a bit more complicated by peeking at the next
>>> few bytes: if they look like a magic number of the next request,
>>> assume the client set the bit accidentally but didn't send a payload
>>> after all; for anything else, assume the client did pass a payload.
>>> But adding in machinery to peek at a prefix is more complex than
>>> either assuming a payload is always present (as done in this patch) or
>>> assuming the bit was in error (and dropping the connection
>>> unconditionally).  Preferences?
>>
>>
>> Ohh, you are right, thanks for comprehensive explanation. I really missed some things you are saying about. Yes, now I agree that "payload always exist when flag is set" is the best effort. Finally, that was our aim of the protocol design: make it more context independent. Probably, we may fix that in specification as preferable or at least possible server behavior about non-compliant client.
> 
> One other possibility I just thought of: have a heuristic where the
> flag set with h->request_length less than 512 bytes is likely to
> indicate an intentional payload (even if for a command where we
> weren't expecting payload, so still a client error); while the flag
> set wtih h->request_length >= 512 bytes is likely to be a mistaken
> setting of the flag (but also still a client error).  NBD_CMD_WRITE is
> probably the only command that will ever need to send a payload larger
> than one sector, but that command already has handling to accept
> payloads of all sizes because we know what to do with them and where
> the client is not in error.
> 

I'd prefer to avoid extra logic for optimizing handling of bad client, better keep code simpler.
Vladimir Sementsov-Ogievskiy Sept. 30, 2023, 1:34 p.m. UTC | #6
On 25.09.23 22:22, Eric Blake wrote:
> Upcoming additions to support NBD 64-bit effect lengths allow for the
> possibility to distinguish between payload length (capped at 32M) and
> effect length (64 bits, although we generally assume 63 bits because
> of off_t limitations).  Without that extension, only the NBD_CMD_WRITE
> request has a payload; but with the extension, it makes sense to allow
> at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
> in a future patch (where the payload is a limited-size struct that in
> turn gives the real effect length as well as a subset of known ids for
> which status is requested).  Other future NBD commands may also have a
> request payload, so the 64-bit extension introduces a new
> NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
> length is a payload length or an effect length, rather than
> hard-coding the decision based on the command.
> 
> According to the spec, a client should never send a command with a
> payload without the negotiation phase proving such extension is
> available.  So in the unlikely event the bit is set or cleared
> incorrectly, the client is already at fault; if the client then
> provides the payload, we can gracefully consume it off the wire and
> fail the command with NBD_EINVAL (subsequent checks for magic numbers
> ensure we are still in sync), while if the client fails to send
> payload we block waiting for it (basically deadlocking our connection
> to the bad client, but not negatively impacting our ability to service
> other clients, so not a security risk).  Note that we do not support
> the payload version of BLOCK_STATUS yet.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> v7: another try at better logic [Vladimir]
> 
> v5: retitled from v4 13/24, rewrite on top of previous patch's switch
> statement [Vladimir]
> 
> v4: less indentation on several 'if's [Vladimir]
> ---
>   nbd/server.c     | 37 +++++++++++++++++++++++++++++++++----
>   nbd/trace-events |  1 +
>   2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 7a6f95071f8..1eabcfc908d 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2322,9 +2322,11 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>                                                  Error **errp)
>   {
>       NBDClient *client = req->client;
> +    bool extended_with_payload;
>       bool check_length = false;
>       bool check_rofs = false;
>       bool allocate_buffer = false;
> +    bool payload_okay = false;
>       unsigned payload_len = 0;
>       int valid_flags = NBD_CMD_FLAG_FUA;
>       int ret;
> @@ -2338,6 +2340,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> 
>       trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
>                                                nbd_cmd_lookup(request->type));
> +    extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
> +        request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
> +    if (extended_with_payload) {
> +        payload_len = request->len;
> +        check_length = true;
> +    }
> +
>       switch (request->type) {
>       case NBD_CMD_DISC:
>           /* Special case: we're going to disconnect without a reply,
> @@ -2354,6 +2363,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>           break;
> 
>       case NBD_CMD_WRITE:
> +        if (client->mode >= NBD_MODE_EXTENDED) {
> +            if (!extended_with_payload) {
> +                /* The client is noncompliant. Trace it, but proceed. */
> +                trace_nbd_co_receive_ext_payload_compliance(request->from,
> +                                                            request->len);
> +            }
> +            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> +        }
> +        payload_okay = true;
>           payload_len = request->len;
>           check_length = true;
>           allocate_buffer = true;
> @@ -2395,6 +2413,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>                      request->len, NBD_MAX_BUFFER_SIZE);
>           return -EINVAL;
>       }
> +    if (payload_len && !payload_okay) {
> +        /*
> +         * For now, we don't support payloads on other commands; but
> +         * we can keep the connection alive by ignoring the payload.
> +         */
> +        assert(request->type != NBD_CMD_WRITE);
> +        request->len = 0;

So, for sure, after this we go to

if (requests->flags & ~valid_flags)... and return -EINVAL.

Why we need to set request->len to 0? Just to not return "operation past EOF" instead of "unsupported flags", if len is too big? Maybe, that worth a comment.

Anyway, I now see nothing wrong in it, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> +    }
>       if (allocate_buffer) {
>           /* READ, WRITE */
>           req->data = blk_try_blockalign(client->exp->common.blk,
> @@ -2405,10 +2431,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>           }
>       }
>       if (payload_len) {
> -        /* WRITE */
> -        assert(req->data);
> -        ret = nbd_read(client->ioc, req->data, payload_len,
> -                       "CMD_WRITE data", errp);
> +        if (payload_okay) {
> +            assert(req->data);
> +            ret = nbd_read(client->ioc, req->data, payload_len,
> +                           "CMD_WRITE data", errp);
> +        } else {
> +            ret = nbd_drop(client->ioc, payload_len, errp);
> +        }
>           if (ret < 0) {
>               return -EIO;
>           }
> diff --git a/nbd/trace-events b/nbd/trace-events
> index f9dccfcfb44..c1a3227613f 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t
>   nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'"
>   nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)"
>   nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received: cookie = %" PRIu64 ", len = %" PRIu64
> +nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64
>   nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32
>   nbd_trip(void) "Reading request"
>
Eric Blake Oct. 5, 2023, 3:38 p.m. UTC | #7
On Mon, Sep 25, 2023 at 02:22:31PM -0500, Eric Blake wrote:
> Upcoming additions to support NBD 64-bit effect lengths allow for the
> possibility to distinguish between payload length (capped at 32M) and
> effect length (64 bits, although we generally assume 63 bits because
> of off_t limitations).
[...]

> +++ b/nbd/server.c
> @@ -2322,9 +2322,11 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>                                                 Error **errp)
>  {
>      NBDClient *client = req->client;
> +    bool extended_with_payload;
>      bool check_length = false;
>      bool check_rofs = false;
>      bool allocate_buffer = false;
> +    bool payload_okay = false;
>      unsigned payload_len = 0;

Pre-existing type mismatch caught as a result of Vladimir's review of
12/12, but:

>      int valid_flags = NBD_CMD_FLAG_FUA;
>      int ret;
> @@ -2338,6 +2340,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> 
>      trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
>                                               nbd_cmd_lookup(request->type));
> +    extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
> +        request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
> +    if (extended_with_payload) {
> +        payload_len = request->len;

this can assign a 64-bit number into a 32-bit variable, which can
truncate to 0,...

> +        check_length = true;
> +    }
> +
>      switch (request->type) {
>      case NBD_CMD_DISC:
>          /* Special case: we're going to disconnect without a reply,
> @@ -2354,6 +2363,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>          break;
> 
>      case NBD_CMD_WRITE:
> +        if (client->mode >= NBD_MODE_EXTENDED) {
> +            if (!extended_with_payload) {
> +                /* The client is noncompliant. Trace it, but proceed. */
> +                trace_nbd_co_receive_ext_payload_compliance(request->from,
> +                                                            request->len);
> +            }
> +            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> +        }
> +        payload_okay = true;
>          payload_len = request->len;

...the pre-existing code is safe only as long as request->len cannot
exceed 32 bytes (which it can't do until later in this series actually
enables extended requests).  Switching the type now is prudent...

>          check_length = true;
>          allocate_buffer = true;
> @@ -2395,6 +2413,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>                     request->len, NBD_MAX_BUFFER_SIZE);
>          return -EINVAL;
>      }
> +    if (payload_len && !payload_okay) {
> +        /*
> +         * For now, we don't support payloads on other commands; but
> +         * we can keep the connection alive by ignoring the payload.
> +         */
> +        assert(request->type != NBD_CMD_WRITE);
> +        request->len = 0;

...otherwise, this check is bypassed for a request size of exactly 4G
if check_length is false and thus the previous conditional for
request->len vs. NBD_MAX_BUFFER_SIZE didn't trigger (prior to this
patch, payload_len was only set for CND_WRITE which also set
check_length).  Thus, I'm squashing in:

diff --git i/nbd/server.c w/nbd/server.c
index 5258064e5ac..1cb66e86a89 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2327,7 +2327,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
     bool check_rofs = false;
     bool allocate_buffer = false;
     bool payload_okay = false;
-    unsigned payload_len = 0;
+    uint64_t payload_len = 0;
     int valid_flags = NBD_CMD_FLAG_FUA;
     int ret;
Vladimir Sementsov-Ogievskiy Oct. 5, 2023, 4:02 p.m. UTC | #8
On 05.10.23 18:38, Eric Blake wrote:
> On Mon, Sep 25, 2023 at 02:22:31PM -0500, Eric Blake wrote:
>> Upcoming additions to support NBD 64-bit effect lengths allow for the
>> possibility to distinguish between payload length (capped at 32M) and
>> effect length (64 bits, although we generally assume 63 bits because
>> of off_t limitations).
> [...]
> 
>> +++ b/nbd/server.c
>> @@ -2322,9 +2322,11 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>>                                                  Error **errp)
>>   {
>>       NBDClient *client = req->client;
>> +    bool extended_with_payload;
>>       bool check_length = false;
>>       bool check_rofs = false;
>>       bool allocate_buffer = false;
>> +    bool payload_okay = false;
>>       unsigned payload_len = 0;
> 
> Pre-existing type mismatch caught as a result of Vladimir's review of
> 12/12, but:
> 
>>       int valid_flags = NBD_CMD_FLAG_FUA;
>>       int ret;
>> @@ -2338,6 +2340,13 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>>
>>       trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
>>                                                nbd_cmd_lookup(request->type));
>> +    extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
>> +        request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
>> +    if (extended_with_payload) {
>> +        payload_len = request->len;
> 
> this can assign a 64-bit number into a 32-bit variable, which can
> truncate to 0,...
> 
>> +        check_length = true;
>> +    }
>> +
>>       switch (request->type) {
>>       case NBD_CMD_DISC:
>>           /* Special case: we're going to disconnect without a reply,
>> @@ -2354,6 +2363,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>>           break;
>>
>>       case NBD_CMD_WRITE:
>> +        if (client->mode >= NBD_MODE_EXTENDED) {
>> +            if (!extended_with_payload) {
>> +                /* The client is noncompliant. Trace it, but proceed. */
>> +                trace_nbd_co_receive_ext_payload_compliance(request->from,
>> +                                                            request->len);
>> +            }
>> +            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
>> +        }
>> +        payload_okay = true;
>>           payload_len = request->len;
> 
> ...the pre-existing code is safe only as long as request->len cannot
> exceed 32 bytes (which it can't do until later in this series actually
> enables extended requests).  Switching the type now is prudent...
> 
>>           check_length = true;
>>           allocate_buffer = true;
>> @@ -2395,6 +2413,14 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>>                      request->len, NBD_MAX_BUFFER_SIZE);
>>           return -EINVAL;
>>       }
>> +    if (payload_len && !payload_okay) {
>> +        /*
>> +         * For now, we don't support payloads on other commands; but
>> +         * we can keep the connection alive by ignoring the payload.
>> +         */
>> +        assert(request->type != NBD_CMD_WRITE);
>> +        request->len = 0;
> 
> ...otherwise, this check is bypassed for a request size of exactly 4G
> if check_length is false and thus the previous conditional for
> request->len vs. NBD_MAX_BUFFER_SIZE didn't trigger (prior to this
> patch, payload_len was only set for CND_WRITE which also set
> check_length).  Thus, I'm squashing in:
> 
> diff --git i/nbd/server.c w/nbd/server.c
> index 5258064e5ac..1cb66e86a89 100644
> --- i/nbd/server.c
> +++ w/nbd/server.c
> @@ -2327,7 +2327,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
>       bool check_rofs = false;
>       bool allocate_buffer = false;
>       bool payload_okay = false;
> -    unsigned payload_len = 0;
> +    uint64_t payload_len = 0;
>       int valid_flags = NBD_CMD_FLAG_FUA;
>       int ret;
> 
> 

OK, agree
diff mbox series

Patch

diff --git a/nbd/server.c b/nbd/server.c
index 7a6f95071f8..1eabcfc908d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2322,9 +2322,11 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
                                                Error **errp)
 {
     NBDClient *client = req->client;
+    bool extended_with_payload;
     bool check_length = false;
     bool check_rofs = false;
     bool allocate_buffer = false;
+    bool payload_okay = false;
     unsigned payload_len = 0;
     int valid_flags = NBD_CMD_FLAG_FUA;
     int ret;
@@ -2338,6 +2340,13 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,

     trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
                                              nbd_cmd_lookup(request->type));
+    extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+        request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
+    if (extended_with_payload) {
+        payload_len = request->len;
+        check_length = true;
+    }
+
     switch (request->type) {
     case NBD_CMD_DISC:
         /* Special case: we're going to disconnect without a reply,
@@ -2354,6 +2363,15 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
         break;

     case NBD_CMD_WRITE:
+        if (client->mode >= NBD_MODE_EXTENDED) {
+            if (!extended_with_payload) {
+                /* The client is noncompliant. Trace it, but proceed. */
+                trace_nbd_co_receive_ext_payload_compliance(request->from,
+                                                            request->len);
+            }
+            valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+        }
+        payload_okay = true;
         payload_len = request->len;
         check_length = true;
         allocate_buffer = true;
@@ -2395,6 +2413,14 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
                    request->len, NBD_MAX_BUFFER_SIZE);
         return -EINVAL;
     }
+    if (payload_len && !payload_okay) {
+        /*
+         * For now, we don't support payloads on other commands; but
+         * we can keep the connection alive by ignoring the payload.
+         */
+        assert(request->type != NBD_CMD_WRITE);
+        request->len = 0;
+    }
     if (allocate_buffer) {
         /* READ, WRITE */
         req->data = blk_try_blockalign(client->exp->common.blk,
@@ -2405,10 +2431,13 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
         }
     }
     if (payload_len) {
-        /* WRITE */
-        assert(req->data);
-        ret = nbd_read(client->ioc, req->data, payload_len,
-                       "CMD_WRITE data", errp);
+        if (payload_okay) {
+            assert(req->data);
+            ret = nbd_read(client->ioc, req->data, payload_len,
+                           "CMD_WRITE data", errp);
+        } else {
+            ret = nbd_drop(client->ioc, payload_len, errp);
+        }
         if (ret < 0) {
             return -EIO;
         }
diff --git a/nbd/trace-events b/nbd/trace-events
index f9dccfcfb44..c1a3227613f 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -71,6 +71,7 @@  nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t
 nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'"
 nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)"
 nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received: cookie = %" PRIu64 ", len = %" PRIu64
+nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64
 nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32
 nbd_trip(void) "Reading request"