diff mbox series

nbd/server: Reject 0-length block status request

Message ID 20180621124937.166549-1-eblake@redhat.com
State New
Headers show
Series nbd/server: Reject 0-length block status request | expand

Commit Message

Eric Blake June 21, 2018, 12:49 p.m. UTC
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(+)

Comments

Vladimir Sementsov-Ogievskiy June 21, 2018, 12:52 p.m. UTC | #1
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,
Eric Blake June 21, 2018, 2:27 p.m. UTC | #2
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.
John Snow June 21, 2018, 9:35 p.m. UTC | #3
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>
diff mbox series

Patch

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,