On Tue, 2019-10-15 at 16:16 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2019 18:07, Eric Blake wrote:
> > On 10/11/19 2:32 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > 11.10.2019 0:00, Eric Blake wrote:
> > > > Qemu as server currently won't accept export names larger than 256
> > > > bytes, nor create dirty bitmap names longer than 1023 bytes, so most
> > > > uses of qemu as client or server have no reason to get anywhere near
> > > > the NBD spec maximum of a 4k limit per string.
> > > >
> > > > However, we weren't actually enforcing things, ignoring when the
> > > > remote side violates the protocol on input, and also having several
> > > > code paths where we send oversize strings on output (for example,
> > > > qemu-nbd --description could easily send more than 4k). Tighten
> > > > things up as follows:
> > > >
> > > > client:
> > > > - Perform bounds check on export name and dirty bitmap request prior
> > > > to handing it to server
> > > > - Validate that copied server replies are not too long (ignoring
> > > > NBD_INFO_* replies that are not copied is not too bad)
> > > > server:
> > > > - Perform bounds check on export name and description prior to
> > > > advertising it to client
> > > > - Reject client name or metadata query that is too long
> > > >
> > > > Signed-off-by: Eric Blake <ebl...@redhat.com>
> > > > ---
> > > > +++ b/include/block/nbd.h
> > > > @@ -232,6 +232,7 @@ enum {
> > > > * going larger would require an audit of more code to make sure we
> > > > * aren't overflowing some other buffer. */
> > >
> > > This comment says, that we restrict export name to 256...
> >
> > Yes, because we still stack-allocate the name in places, but 4k is too
> > large for stack allocation. But we're inconsistent on where we use the
> > smaller 256-limit; the server won't serve an image
> > that large, but doesn't prevent a client from requesting a 4k name export
> > (even though that export will not be present).
> >
> >
> > > > +++ b/blockdev-nbd.c
> > > > @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool
> > > > has_name, const char *name,
> > > > name = device;
> > > > }
> > > >
> > > > + if (strlen(name) > NBD_MAX_STRING_SIZE) {
> > > > + error_setg(errp, "export name '%s' too long", name);
> > > > + return;
> > > > + }
> > >
> > > Hmmm, no, so here we restrict to 4096, but, we will not allow client to
> > > request more than
> > > 256. Seems, to correctly update server-part, we should drop
> > > NBD_MAX_NAME_SIZE and do the
> > > audit mentioned in the comment above its definition.
> >
> > Yeah, I guess it's time to just get rid of NBD_MAX_NAME_SIZE, and move away
> > from stack allocations. Should I do that as a followup to this patch, or
> > spin a v3?
>
> Hmm. It's OK too.
>
> With
> - fixed mem-leak in nbd_process_options
> - s/x_dirty_bitmap/x-dirty-bitmap in nbd_process_options in error message
> - following yours new wordings
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>
> However, this patch introduces possible crash point, asserting on bitmap name
> below, so it would better
> be fixed before this patch or immediately after it.. Still, it's unlikely to
> have a bitmap with name
> longer than 4k..
>
> >
> > > > +++ b/nbd/client.c
> > > > @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char
> > > > **name, char **description,
> > > > return -1;
> > > > }
> > > > len -= sizeof(namelen);
> > > > - if (len < namelen) {
> > > > - error_setg(errp, "incorrect option name length");
> > > > + if (len < namelen || namelen > NBD_MAX_STRING_SIZE) {
> > > > + error_setg(errp, "incorrect list name length");
> > >
> > > New wording made me go above and read the comment, what functions does.
> > > Comment is good, but without
> > > it, it sounds like name of the list for me...
> >
> > Maybe:
> >
> > incorrect name length in server's list response
>
> Yes, this is better, thanks
>
> >
> > >
> > > > nbd_send_opt_abort(ioc);
> > > > return -1;
> > > > }
> > > > @@ -303,6 +303,11 @@ static int nbd_receive_list(QIOChannel *ioc, char
> > > > **name, char **description,
> > > > local_name[namelen] = '\0';
> > > > len -= namelen;
> > > > if (len) {
> > > > + if (len > NBD_MAX_STRING_SIZE) {
> > > > + error_setg(errp, "incorrect list description length");
> >
> > and
> >
> > incorrect description length in server's list response
> >
> >
> > > > @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc,
> > > > uint32_t opt,
> > > > if (query) {
> > > > query_len = strlen(query);
> > > > data_len += sizeof(query_len) + query_len;
> > > > + assert(query_len <= NBD_MAX_STRING_SIZE);
> > > > } else {
> > > > assert(opt == NBD_OPT_LIST_META_CONTEXT);
> > > > }
> > >
> > > you may assert export_len as well..
> >
> > It was asserted earlier, but doing it again might not hurt, especially if I
> > do the followup patch getting rid of NBD_MAX_NAME_SIZE
> >
> >
> > > > @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
> > > > uint64_t dev_offset,
> > > > exp->export_bitmap = bm;
> > > > exp->export_bitmap_context =
> > > > g_strdup_printf("qemu:dirty-bitmap:%s",
> > > > bitmap);
> > > > + /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */
> > >
> > > Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps.
> > > But for non-persistent
> > > name length is actually unlimited. So, we should either limit all bitmap
> > > names to 1023 (hope,
> > > this will not break existing scenarios) or error out here (or earlier)
> > > instead of assertion.
> >
> > I'm leaning towards limiting ALL bitmaps to the same length (as we've
> > already debated the idea of being able to convert an existing bitmap from
> > transient to persistent).
>
> Agreed, but ..
>
> >
> > >
> > > We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE <
> > > BME_MAX_NAME_SIZE + sizeof("qemu:dirty-bitmap:") - 1)
> >
> > Except that BME_MAX_NAME_SIZE is not (currently) in a public .h file.
> >
>
> .. I think, than it should be new BLOCK_DIRTY_BITMAP_MAX_NAME_SIZE.. And
> we'll have to note it in qapi doc..
> Should this change go through deprecation? Or we consider non-persistent
> bitmaps as something not really useful?
>
> --
> Best regards,
> Vladimir
I followed upon the new patch and the comments, and it seems ok now to me,
(including the comments that were already made) but I haven't
checked if there are more cases of missing length checks.
Best regards,
Maxim Levitsky
