[Qemu-devel] [PATCH] nbd/server: Reject 0-length block status request

[Qemu-devel] [PATCH] nbd/server: Reject 0-length block status request

[Eric Blake]

The NBD spec says that behavior is unspecified if the client
requests 0 length for block status; but since the structured
reply is documenting as returning a non-zero length, it's
easier to just diagnose this with an EINVAL error than to
figure out what to return.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index 9e1f2271784..493a926e063 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2007,6 +2007,10 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
                                       "discard failed", errp);

     case NBD_CMD_BLOCK_STATUS:
+        if (!request->len) {
+            return nbd_send_generic_reply(client, request->handle, -EINVAL,
+                                          "need non-zero length", errp);
+        }
         if (client->export_meta.valid && client->export_meta.base_allocation) {
             return nbd_co_send_block_status(client, request->handle,
                                             blk_bs(exp->blk), request->from,
-- 
2.14.4

Re: [Qemu-devel] [PATCH] nbd/server: Reject 0-length block status request

[Vladimir Sementsov-Ogievskiy]

21.06.2018 15:49, Eric Blake wrote:
> The NBD spec says that behavior is unspecified if the client
> requests 0 length for block status; but since the structured
> reply is documenting as returning a non-zero length, it's
> easier to just diagnose this with an EINVAL error than to
> figure out what to return.
>
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>

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

> ---
>   nbd/server.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 9e1f2271784..493a926e063 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2007,6 +2007,10 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>                                         "discard failed", errp);
>
>       case NBD_CMD_BLOCK_STATUS:
> +        if (!request->len) {
> +            return nbd_send_generic_reply(client, request->handle, -EINVAL,
> +                                          "need non-zero length", errp);
> +        }
>           if (client->export_meta.valid && client->export_meta.base_allocation) {
>               return nbd_co_send_block_status(client, request->handle,
>                                               blk_bs(exp->blk), request->from,


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] nbd/server: Reject 0-length block status request

[Eric Blake]

On 06/21/2018 07:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> 21.06.2018 15:49, Eric Blake wrote:
>> The NBD spec says that behavior is unspecified if the client
>> requests 0 length for block status; but since the structured
>> reply is documenting as returning a non-zero length, it's
>> easier to just diagnose this with an EINVAL error than to
>> figure out what to return.
>>
>> CC: qemu-stable@nongnu.org
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks; added to my NBD queue.  Doing this makes it easier to add the 
assertion necessary to shut up gcc warning about an uninitialized 
variable in the other patches ready to pull.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] nbd/server: Reject 0-length block status request

[John Snow]

On 06/21/2018 08:49 AM, Eric Blake wrote:
> The NBD spec says that behavior is unspecified if the client
> requests 0 length for block status; but since the structured
> reply is documenting as returning a non-zero length, it's
> easier to just diagnose this with an EINVAL error than to
> figure out what to return.
> 

Relevant section:

REQUEST TYPES / NBD_CMD_BLOCK_STATUS (7)

"A block status query request. Length and offset define the range of
interest. The client SHOULD NOT request a status length of 0; the
behavior of a server on such a request is unspecified although the
server SHOULD NOT disconnect."

Leave a little breadcrumb in the commit message because it's headed to
-stable.

> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 9e1f2271784..493a926e063 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2007,6 +2007,10 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>                                        "discard failed", errp);
> 
>      case NBD_CMD_BLOCK_STATUS:
> +        if (!request->len) {
> +            return nbd_send_generic_reply(client, request->handle, -EINVAL,
> +                                          "need non-zero length", errp);
> +        }
>          if (client->export_meta.valid && client->export_meta.base_allocation) {
>              return nbd_co_send_block_status(client, request->handle,
>                                              blk_bs(exp->blk), request->from,
> 

Looks correct assuming spec agrees.

Reviewed-by: John Snow <jsnow@redhat.com>