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 <dgilb...@redhat.com> > > Signed-off-by: Eric Blake <ebl...@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); -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org