The NBD spec suggests that a server should never advertise a size
inconsistent with its minimum block alignment, as that tail is
effectively inaccessible to a compliant client obeying those block
constraints. Since we have a habit of rounding up rather than
truncating, to avoid losing the last few bytes of user input, and we
cannot access the tail when the server advertises bogus block sizing,
abort the connection to alert the server to fix their bug.  And
rejecting such servers matches what we already did for a min_block
that was not a power of 2 or which was larger than max_block.

Does not impact either qemu (which always sends properly aligned
sizes) or nbdkit (which does not send minimum block requirements yet);
so this is mostly aimed at new NBD server implementations, and ensures
that the rest of our code can assume the size is aligned.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190329163407.26919-1-eblake@redhat.com>
---

This is the rewrite of the original 7/6 that I posted, retitled and
hoisted earlier in the series (since Vladimir was right that 4/6
depends on it). I've added patches 1-3 to my NBD queue for a pull
request on Monday as they now have R-b; I'd love to also have enough
reviews for 3.5, 4, 5, 6, and also another thread on fixing 'qemu-img
info' output, to include those as well.

 nbd/client.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/nbd/client.c b/nbd/client.c
index de7da48246b..9f5eb79930c 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -426,6 +426,14 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
+            if (info->min_block &&
+                !QEMU_IS_ALIGNED(info->size, info->min_block)) {
+                error_setg(errp, "server size %" PRIu64 "is not multiple of "
+                           "minimum block size %" PRIu32, info->size,
+                           info->min_block);
+                nbd_send_opt_abort(ioc);
+                return -1;
+            }
             trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
             break;

-- 
2.20.1

30.03.2019 18:57, Eric Blake wrote:
> The NBD spec suggests that a server should never advertise a size
> inconsistent with its minimum block alignment, as that tail is
> effectively inaccessible to a compliant client obeying those block
> constraints. Since we have a habit of rounding up rather than
> truncating, to avoid losing the last few bytes of user input, and we
> cannot access the tail when the server advertises bogus block sizing,
> abort the connection to alert the server to fix their bug.  And
> rejecting such servers matches what we already did for a min_block
> that was not a power of 2 or which was larger than max_block.
> 
> Does not impact either qemu (which always sends properly aligned
> sizes) or nbdkit (which does not send minimum block requirements yet);
> so this is mostly aimed at new NBD server implementations, and ensures
> that the rest of our code can assume the size is aligned.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20190329163407.26919-1-eblake@redhat.com>
> ---
> 
> This is the rewrite of the original 7/6 that I posted, retitled and
> hoisted earlier in the series (since Vladimir was right that 4/6
> depends on it). I've added patches 1-3 to my NBD queue for a pull
> request on Monday as they now have R-b; I'd love to also have enough
> reviews for 3.5, 4, 5, 6, and also another thread on fixing 'qemu-img
> info' output, to include those as well.
> 
>   nbd/client.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index de7da48246b..9f5eb79930c 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -426,6 +426,14 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
>                   nbd_send_opt_abort(ioc);
>                   return -1;
>               }
> +            if (info->min_block &&
> +                !QEMU_IS_ALIGNED(info->size, info->min_block)) {
> +                error_setg(errp, "server size %" PRIu64 "is not multiple of "

s/server/export/ I think

> +                           "minimum block size %" PRIu32, info->size,
> +                           info->min_block);
> +                nbd_send_opt_abort(ioc);
> +                return -1;
> +            }
>               trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
>               break;
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir