Re: [PATCH v6 06/17] nbd/server: Support a request payload

From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [PATCH v6 06/17] nbd/server: Support a request payload
Date: Fri, 8 Sep 2023 12:43:41 -0500	[thread overview]
Message-ID: <ljj22s32exaqo4e4gkieryjhfhcb5qkuh2ctbbmw6up2wkzwbl@ffcyqythqcdo> (raw)
In-Reply-To: <yyoofxrmbw4g3w7ypyba3afwy6sck6d4gbv57g7wagrv75xuoj@hso5ii2sm2gg>

On Wed, Sep 06, 2023 at 12:52:22PM -0500, Eric Blake wrote:
> On Tue, Sep 05, 2023 at 05:36:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 29.08.23 20:58, 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; although a client
> > > should never send a command with a payload without the negotiation
> > > phase proving such extension is available, we are now able to
> > > gracefully fail unexpected client payloads while keeping the
> > > connection alive.  Note that we do not support the payload version of
> > > BLOCK_STATUS yet.
> > > 
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > > 
> > > 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     | 33 ++++++++++++++++++++++++++++-----
> > >   nbd/trace-events |  1 +
> > >   2 files changed, 29 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/nbd/server.c b/nbd/server.c
> > > index dd3ab59224c..adcfcdeacb7 100644
> > > --- a/nbd/server.c
> > > +++ b/nbd/server.c
> > > @@ -2334,7 +2334,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> > >                                                  Error **errp)
> > >   {
> > >       NBDClient *client = req->client;
> > > -    bool check_length = false;
> > > +    bool extended_with_payload;
> > > +    bool check_length;
> > >       bool check_rofs = false;
> > >       bool allocate_buffer = false;
> > >       unsigned payload_len = 0;
> > > @@ -2350,6 +2351,9 @@ 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));
> > > +    check_length = extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
> > > +        request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
> > > +
> > >       switch (request->type) {
> > >       case NBD_CMD_DISC:
> > >           /* Special case: we're going to disconnect without a reply,
> > > @@ -2366,6 +2370,14 @@ 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_len = request->len;
> > >           check_length = true;
> > >           allocate_buffer = true;
> > > @@ -2407,6 +2419,15 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
> > 
> > more context:
> > 
> >     /* Payload and buffer handling. */
> >     if (!payload_len) {
> >         req->complete = true;
> 
> At this point, payload_len is equal to 0 for all but NBD_CMD_WRITE.  [1]
> 
> >     }
> >     if (check_length && request->len > NBD_MAX_BUFFER_SIZE) {
> >         /* READ, WRITE, CACHE */
> >         error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
> >                    request->len, NBD_MAX_BUFFER_SIZE);
> >         return -EINVAL;
> >     }
> > 
> > 
> > > +    if (extended_with_payload && !allocate_buffer) {
> > 
> > it's correct but strange, as allocate_buffer is (READ or WRITE), and READ is totally unrelated here.
> 
> Oh, you do have a point.  If a client mistakenly passes the
> extended_with_payload flag on NBD_CMD_READ, we end up skipping this
> code which tries to parse off that payload, meaning we could be out of
> sync for reacting to the next command; if the client is particularly
> malicious, they could send payload that resembles another valid
> command.  Checking specifically for !WRITE rather than for
> !allocate_buffer is more accurate, although I was trying to avoid
> duplicating checks that were covered in the switch statement above.
> 
> I'll have to think about this a bit.
> 
> > 
> > > +        /*
> > > +         * For now, we don't support payloads on other commands; but
> > > +         * we can keep the connection alive by ignoring the payload.
> > 
> > Was it in specification, that we can safely ignore unknown payload for known commands?
> 
> Well, at this point, the client is already non-compliant, so anything
> we do is best effort only (the client may have set the bit but not
> sent any payload, at which case we block until the client does send
> another command but we consume that command as payload; or it sent
> more payload than the size it advertises and so we see a magic number
> mismatch when trying to interpret those extra bytes as the next
> command).
> 
> But yes, the current spec wording mentions consuming payload bytes and
> then failing with NBD_EINVAL.  Is there a potential that we need to
> change the spec wording, if we think a malicious client can abuse the
> flag to force the server into a deadlock scenario which can be abused
> as a denial-of-service attack against other clients, compared to the
> more conservative approach of just disconnecting because the client
> sent a bad flag?  Generally, I look at disconnecting in response to
> client behavior as the least favorable response to take; attempting to
> return an error message is nicer.  And being blocked on I/O is not
> necessarily a denial of service (it isn't actively consuming CPU
> cycles, and we don't stop servicing parallel clients).
> 
> > 
> > > +         */
> > > +        assert(request->type != NBD_CMD_WRITE);
> > > +        payload_len = request->len;
> > 
> > what I don't like here, is that we already set req->complete to true for this request, when payload_len was zero.
> > 
> > Probably, for extended_with_payload requests we should initialize payload_len at top of the function?
> 
> Well, we DO initialize it to 0, and then assign it under NBD_CMD_READ.
> But the code that sets req->complete should probably NOT be setting it
> if we are about to attempt reading payload bytes; that is, back at
> [1], the code there probably needs to be conditional on
> extended_with_payload.
> 
> > 
> > > +        request->len = 0;
> > > +    }
> > >       if (allocate_buffer) {>           /* READ, WRITE */
> > >           req->data = blk_try_blockalign(client->exp->common.blk,
> > > @@ -2417,10 +2438,12 @@ 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 (req->data) {
> > 
> > and req->data is actually (READ or WRITE) ( == allocated_buffer) as well.
> > 
> > I'd prefer here "if (request->type == NBD_CMD_WRITE)" here
> 
> Looks like I'll be respinning this patch slightly.

Here's what I'm planning to squash in.  Your idea of setting
payload_len up front makes sense.  I will also have to squash in some
followups to 17/17, when I start accepting payload for
NBD_CMD_BLOCK_STATUS.

diff --git i/nbd/server.c w/nbd/server.c
index d54dd0604bf..35d3f8989f2 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2317,36 +2317,41 @@ static int coroutine_fn nbd_co_send_bitmap(NBDClient *client,
  * to the client (although the caller may still need to disconnect after
  * reporting the error).
  */
 static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
                                                NBDRequest *request,
                                                Error **errp)
 {
     NBDClient *client = req->client;
     bool extended_with_payload;
-    bool check_length;
+    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;

     g_assert(qemu_in_coroutine());
     assert(client->recv_coroutine == qemu_coroutine_self());
     ret = nbd_receive_request(client, request, errp);
     if (ret < 0) {
         return ret;
     }

     trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
                                              nbd_cmd_lookup(request->type));
-    check_length = extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+    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,
          * whether or not flags, from, or len are bogus */
         req->complete = true;
         return -EIO;

     case NBD_CMD_READ:
@@ -2360,18 +2365,19 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
     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;
         check_rofs = true;
         break;

     case NBD_CMD_FLUSH:
         break;

@@ -2401,38 +2407,38 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req,
     if (!payload_len) {
         req->complete = true;
     }
     if (check_length && request->len > NBD_MAX_BUFFER_SIZE) {
         /* READ, WRITE, CACHE */
         error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
                    request->len, NBD_MAX_BUFFER_SIZE);
         return -EINVAL;
     }
-    if (extended_with_payload && !allocate_buffer) {
+    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);
-        payload_len = request->len;
         request->len = 0;
     }
     if (allocate_buffer) {
         /* READ, WRITE */
         req->data = blk_try_blockalign(client->exp->common.blk,
                                        request->len);
         if (req->data == NULL) {
             error_setg(errp, "No memory");
             return -ENOMEM;
         }
     }
     if (payload_len) {
-        if (req->data) {
+        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;
         }
         req->complete = true;


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org


