[PATCH] nbd/server: Use smarter assert

[PATCH] nbd/server: Use smarter assert

[Eric Blake]

Assigning strlen() to a uint32_t and then asserting that it isn't too
large doesn't catch the case of an input string 4G in length.
Thankfully, the incoming string can never be that large: if the export
name is reflecting what the client asked about, we already guarantee
that we drop the NBD connection if the client tries to send more than
32M in a single NBD_OPT_* request; and if the export name is coming
from qemu, nbd_receive_negotiate() asserted that strlen(info->name) <=
NBD_MAX_STRING_SIZE.  Still, it doesn't hurt to be more explicit in
how we write our assertion that we are aware that no wraparound is
possible.

Fixes: 93676c88 ("nbd: Don't send oversize strings", v4.2.0)
Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/client.c b/nbd/client.c
index 60c9f4941a..b601ee97e5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -658,7 +658,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
     char *p;

     data_len = sizeof(export_len) + export_len + sizeof(queries);
-    assert(export_len <= NBD_MAX_STRING_SIZE);
+    assert(strlen(export) <= NBD_MAX_STRING_SIZE);
     if (query) {
         query_len = strlen(query);
         data_len += sizeof(query_len) + query_len;
-- 
2.37.3

Re: [PATCH] nbd/server: Use smarter assert

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> Assigning strlen() to a uint32_t and then asserting that it isn't too
> large doesn't catch the case of an input string 4G in length.
> Thankfully, the incoming string can never be that large: if the export
> name is reflecting what the client asked about, we already guarantee
> that we drop the NBD connection if the client tries to send more than
> 32M in a single NBD_OPT_* request; and if the export name is coming
> from qemu, nbd_receive_negotiate() asserted that strlen(info->name) <=
> NBD_MAX_STRING_SIZE.  Still, it doesn't hurt to be more explicit in
> how we write our assertion that we are aware that no wraparound is
> possible.
> 
> Fixes: 93676c88 ("nbd: Don't send oversize strings", v4.2.0)
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  nbd/client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 60c9f4941a..b601ee97e5 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -658,7 +658,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
>      char *p;
> 
>      data_len = sizeof(export_len) + export_len + sizeof(queries);
> -    assert(export_len <= NBD_MAX_STRING_SIZE);
> +    assert(strlen(export) <= NBD_MAX_STRING_SIZE);
>      if (query) {
>          query_len = strlen(query);
>          data_len += sizeof(query_len) + query_len;
> -- 
> 2.37.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] nbd/server: Use smarter assert

[Eric Blake]

Given the file touched by this patch[1],

The subject should use 'nbd/client:'

On Mon, Oct 17, 2022 at 12:37:27PM -0500, Eric Blake wrote:
> Assigning strlen() to a uint32_t and then asserting that it isn't too
> large doesn't catch the case of an input string 4G in length.
> Thankfully, the incoming string can never be that large: if the export
> name is reflecting what the client asked about, we already guarantee

and this should be 'is reflecting a name provided by the server'...

> that we drop the NBD connection if the client tries to send more than
> 32M in a single NBD_OPT_* request; and if the export name is coming

...'if the server tries to send more than 32M in a reply to a single
NBD_OPT_* request from the client'

> from qemu, nbd_receive_negotiate() asserted that strlen(info->name) <=
> NBD_MAX_STRING_SIZE.  Still, it doesn't hurt to be more explicit in
> how we write our assertion that we are aware that no wraparound is
> possible.
> 
> Fixes: 93676c88 ("nbd: Don't send oversize strings", v4.2.0)
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/client.c | 2 +-

[1] this patch is to the client, not the server.

>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 60c9f4941a..b601ee97e5 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -658,7 +658,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
>      char *p;
> 
>      data_len = sizeof(export_len) + export_len + sizeof(queries);
> -    assert(export_len <= NBD_MAX_STRING_SIZE);
> +    assert(strlen(export) <= NBD_MAX_STRING_SIZE);
>      if (query) {
>          query_len = strlen(query);
>          data_len += sizeof(query_len) + query_len;

           assert(query_len <= NBD_MAX_STRING_SIZE);

and this assertion on query_len could use the same treatment (similar
analysis as to why callers are never passing in a 4G string, but it
doesn't hurt to be explicit in the assertion).  v2 coming up.

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