| Message ID | 20221017191207.1255807-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] nbd/client: 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 strings can never be that large: if the > export name or query is reflecting a string the client got from the > server, we already guarantee that we dropped the NBD connection if the > server sent more than 32M in a single reply to our NBD_OPT_* request; > if the export name is coming from qemu, nbd_receive_negotiate() > asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and > similarly, a query string via x->dirty_bitmap coming from the user was > bounds-checked in either qemu-nbd or by the limitations of QMP. > Still, it doesn't hurt to be more explicit in how we write our > assertions to not have to analyze whether inadvertent 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> > --- > > v2: update subject line and commit message to reflect file being > touched; adjust a second nearby assertion with the same issue > > nbd/client.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 30d5383cb1..90a6b7b38b 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -658,11 +658,11 @@ 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); > + assert(strlen(query) <= NBD_MAX_STRING_SIZE); > } else { > assert(opt == NBD_OPT_LIST_META_CONTEXT); > } > -- > 2.37.3 >
On 17/10/22 21:12, 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 strings can never be that large: if the > export name or query is reflecting a string the client got from the > server, we already guarantee that we dropped the NBD connection if the > server sent more than 32M in a single reply to our NBD_OPT_* request; > if the export name is coming from qemu, nbd_receive_negotiate() > asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and > similarly, a query string via x->dirty_bitmap coming from the user was > bounds-checked in either qemu-nbd or by the limitations of QMP. > Still, it doesn't hurt to be more explicit in how we write our > assertions to not have to analyze whether inadvertent 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> > --- > > v2: update subject line and commit message to reflect file being > touched; adjust a second nearby assertion with the same issue > > nbd/client.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 30d5383cb1..90a6b7b38b 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -658,11 +658,11 @@ 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); > + assert(strlen(query) <= NBD_MAX_STRING_SIZE); > } else { > assert(opt == NBD_OPT_LIST_META_CONTEXT); > } Nitpicking (pre-existing) the assertions could be moved before the assignations. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 10/17/22 22:12, 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 strings can never be that large: if the > export name or query is reflecting a string the client got from the > server, we already guarantee that we dropped the NBD connection if the > server sent more than 32M in a single reply to our NBD_OPT_* request; > if the export name is coming from qemu, nbd_receive_negotiate() > asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and > similarly, a query string via x->dirty_bitmap coming from the user was > bounds-checked in either qemu-nbd or by the limitations of QMP. > Still, it doesn't hurt to be more explicit in how we write our > assertions to not have to analyze whether inadvertent 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> > --- > > v2: update subject line and commit message to reflect file being > touched; adjust a second nearby assertion with the same issue > > nbd/client.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 30d5383cb1..90a6b7b38b 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -658,11 +658,11 @@ 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); > + assert(strlen(query) <= NBD_MAX_STRING_SIZE); > } else { > assert(opt == NBD_OPT_LIST_META_CONTEXT); > } I'm a bit late, and this should work as is. Still, for me it's a bit strange: you point to the fact that we probably overflow uint32_t variable. But we keep this fact hidden in the code. So, everyone who read should guess "aha, this extra strlen here is because the previous one may overflow the variable)". Could we use strnlen() instead of strlen()? That would be also more effective.
On Mon, Oct 24, 2022 at 02:59:48PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 10/17/22 22:12, 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 strings can never be that large: if the > > export name or query is reflecting a string the client got from the > > server, we already guarantee that we dropped the NBD connection if the > > server sent more than 32M in a single reply to our NBD_OPT_* request; > > if the export name is coming from qemu, nbd_receive_negotiate() > > asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and > > similarly, a query string via x->dirty_bitmap coming from the user was > > bounds-checked in either qemu-nbd or by the limitations of QMP. > > Still, it doesn't hurt to be more explicit in how we write our > > assertions to not have to analyze whether inadvertent 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> > > --- > > > > v2: update subject line and commit message to reflect file being > > touched; adjust a second nearby assertion with the same issue > > > > nbd/client.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/nbd/client.c b/nbd/client.c > > index 30d5383cb1..90a6b7b38b 100644 > > --- a/nbd/client.c > > +++ b/nbd/client.c > > @@ -658,11 +658,11 @@ 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); > > + assert(strlen(query) <= NBD_MAX_STRING_SIZE); > > } else { > > assert(opt == NBD_OPT_LIST_META_CONTEXT); > > } > > I'm a bit late, and this should work as is. > > Still, for me it's a bit strange: you point to the fact that we probably overflow uint32_t variable. But we keep this fact hidden in the code. So, everyone who read should guess "aha, this extra strlen here is because the previous one may overflow the variable)". > > Could we use strnlen() instead of strlen()? That would be also more effective. Good idea. As in: assert(strnlen(query, NBD_MAX_STRING_SIZE + 1) <= NBD_MAX_STRING_SIZE);
diff --git a/nbd/client.c b/nbd/client.c index 30d5383cb1..90a6b7b38b 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -658,11 +658,11 @@ 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); + assert(strlen(query) <= NBD_MAX_STRING_SIZE); } else { assert(opt == NBD_OPT_LIST_META_CONTEXT); }
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 strings can never be that large: if the export name or query is reflecting a string the client got from the server, we already guarantee that we dropped the NBD connection if the server sent more than 32M in a single reply to our NBD_OPT_* request; if the export name is coming from qemu, nbd_receive_negotiate() asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and similarly, a query string via x->dirty_bitmap coming from the user was bounds-checked in either qemu-nbd or by the limitations of QMP. Still, it doesn't hurt to be more explicit in how we write our assertions to not have to analyze whether inadvertent 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> --- v2: update subject line and commit message to reflect file being touched; adjust a second nearby assertion with the same issue nbd/client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)