| Message ID | 20221017173727.1246179-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd/server: Use smarter assert | expand |
* 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 >
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.
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;
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(-)