Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/2] nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

[John Snow]

On 07/17/2017 03:26 PM, Eric Blake wrote:
> A typo in commit 23e099c set the size of buf[] used in response
> to NBD_OPT_EXPORT_NAME according to the length needed for old-style
> negotiation (4 bytes of flag information) instead of the intended
> 2 bytes used in new style.  If the client doesn't enable
> NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many,
> and is then out of sync in response to the client's next command
> (the bug is masked when modern qemu is the client, since we enable
> the no zeroes flag).
> 
> While touching this code, add some more defines to nbd_internal.h
> rather than having quite so many magic numbers in the .c; also,
> use "" initialization rather than memset(), and tweak the oldstyle
> negotiation to better match the spec description of the layout
> (since the spec is big-endian, skipping two bytes as 0 followed by
> writing a 2-byte flag is the same as writing a zero-extended 4-byte
> flag), to make it a bit easier to follow compared to the spec.
> 
> [checkpatch.pl has some false positives in the comments]
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v2: defines rather than magic numbers [John]
> ---
>  nbd/nbd-internal.h |  8 ++++++--
>  nbd/server.c       | 18 ++++++++++--------
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index 4065bc68ac..396ddb4d3e 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -38,9 +38,13 @@
>   */
> 
>  /* Size of all NBD_OPT_*, without payload */
> -#define NBD_REQUEST_SIZE        (4 + 2 + 2 + 8 + 8 + 4)
> +#define NBD_REQUEST_SIZE            (4 + 2 + 2 + 8 + 8 + 4)
>  /* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without payload */
> -#define NBD_REPLY_SIZE          (4 + 4 + 8)
> +#define NBD_REPLY_SIZE              (4 + 4 + 8)
> +/* Size of reply to NBD_OPT_EXPORT_NAME */
> +#define NBD_REPLY_EXPORT_NAME_SIZE  (8 + 2 + 124)
> +/* Size of oldstyle negotiation */
> +#define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124)
> 
>  #define NBD_REQUEST_MAGIC       0x25609513
>  #define NBD_REPLY_MAGIC         0x67446698
> diff --git a/nbd/server.c b/nbd/server.c
> index 49ed57455c..82a78bf439 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -283,12 +283,16 @@ static int nbd_negotiate_handle_export_name(NBDClient 
> *client, uint32_t length,
>                                              Error **errp)
>  {
>      char name[NBD_MAX_NAME_SIZE + 1];
> -    char buf[8 + 4 + 124] = "";
> +    char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
>      size_t len;
>      int ret;
> 
>      /* Client sends:
>          [20 ..  xx]   export name (length bytes)
> +       Server replies:
> +        [ 0 ..   7]   size
> +        [ 8 ..   9]   export flags
> +        [10 .. 133]   reserved     (0) [unless no_zeroes]

Alright, there's the 8, 2, 124.

>       */
>      trace_nbd_negotiate_handle_export_name();
>      if (length >= sizeof(name)) {
> @@ -800,22 +804,21 @@ static int nbd_negotiate_options(NBDClient *client, 
> uint16_t myflags,
>   */
>  static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>  {
> -    char buf[8 + 8 + 8 + 128];
> +    char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = "";

huh, TIL.

>      int ret;
>      const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
>                                NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
>                                NBD_FLAG_SEND_WRITE_ZEROES);
>      bool oldStyle;
> 
> -    /* Old style negotiation header without options
> +    /* Old style negotiation header, no room for options
>          [ 0 ..   7]   passwd       ("NBDMAGIC")
>          [ 8 ..  15]   magic        (NBD_CLIENT_MAGIC)
>          [16 ..  23]   size
> -        [24 ..  25]   server flags (0)
> -        [26 ..  27]   export flags
> +        [24 ..  27]   export flags (zero-extended)
>          [28 .. 151]   reserved     (0)

And there's the 8, 8, 8, 4, 124.

> 
> -       New style negotiation header with options
> +       New style negotiation header, client can send options
>          [ 0 ..   7]   passwd       ("NBDMAGIC")
>          [ 8 ..  15]   magic        (NBD_OPTS_MAGIC)
>          [16 ..  17]   server flags (0)
> @@ -825,7 +828,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, 
> Error **errp)
>      qio_channel_set_blocking(client->ioc, false, NULL);
> 
>      trace_nbd_negotiate_begin();
> -    memset(buf, 0, sizeof(buf));
>      memcpy(buf, "NBDMAGIC", 8);
> 
>      oldStyle = client->exp != NULL && !client->tlscreds;
> @@ -834,7 +836,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, 
> Error **errp)
>                                        client->exp->nbdflags | myflags);
>          stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
>          stq_be_p(buf + 16, client->exp->size);
> -        stw_be_p(buf + 26, client->exp->nbdflags | myflags);
> +        stl_be_p(buf + 24, client->exp->nbdflags | myflags);

Makes sense to me.

> 
>          if (nbd_write(client->ioc, buf, sizeof(buf), errp) < 0) {
>              error_prepend(errp, "write failed: ");
> 

Thanks,

Reviewed-by: John Snow <address@hidden>