Re: [PATCH v2 1/2] nbd: Don't send oversize strings
On 10/15/19 11:16 AM, Vladimir Sementsov-Ogievskiy wrote:
@@ -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 seriously doubting that any existing scenarios try to use a name
that long. If no one was relying on a long name (especially since it was
inconsistent between persistent being limited to qcow2 constraints and
non-persistent having no limit), we can consider it as a bug-fix rather
than something needing a deprecation period.
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?
I'm preparing a v3 patch that just goes ahead and adds the limit on
bitmap names everywhere, as a separate patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 1/2] nbd: Don't send oversize strings
On Tue, 2019-10-15 at 16:16 +, 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
> > > > ---
> > > > +++ 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
>
> 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
Re: [PATCH v2 1/2] nbd: Don't send oversize strings
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
>>> ---
>
>>> +++ 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
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
Re: [PATCH v2 1/2] nbd: Don't send oversize strings
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
---
+++ 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?
+++ 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
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).
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 1/2] nbd: Don't send oversize strings
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
> ---
> include/block/nbd.h | 1 +
> block/nbd.c | 9 +
> blockdev-nbd.c | 5 +
> nbd/client.c| 16 +---
> nbd/server.c| 14 --
> qemu-nbd.c | 9 +
> 6 files changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd705a9e4..fcabdf0f37c3 100644
> --- a/include/block/nbd.h
> +++ 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...
> #define NBD_MAX_NAME_SIZE 256
> +#define NBD_MAX_STRING_SIZE 4096
>
> /* Two types of reply structures */
> #define NBD_SIMPLE_REPLY_MAGIC 0x67446698
> diff --git a/block/nbd.c b/block/nbd.c
> index 813c40d8f067..76eb1dbe04df 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1621,6 +1621,10 @@ static int nbd_process_options(BlockDriverState *bs,
> QDict *options,
> }
>
> s->export = g_strdup(qemu_opt_get(opts, "export"));
> +if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) {
> +error_setg(errp, "export name too long to send to server");
> +goto error;
> +}
>
> s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
> if (s->tlscredsid) {
> @@ -1638,6 +1642,11 @@ static int nbd_process_options(BlockDriverState *bs,
> QDict *options,
> }
>
> s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap"));
> +if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) >
> NBD_MAX_STRING_SIZE) {
> +error_setg(errp, "x_dirty_bitmap query too long to send to server");
> +goto error;
this is new latest "goto error", you should add g_free(s->x_dirty_bitmap) in
following "if (ret < 0)"
> +}
> +
> s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
>
> ret = 0;
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 6a8b206e1d74..8c20baa4a4b9 100644
> --- a/blockdev-nbd.c
> +++ 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.
> +
> if (nbd_export_find(name)) {
> error_setg(errp, "NBD server already has export named '%s'", name);
> return;
> diff --git a/nbd/client.c b/nbd/client.c
> index f6733962b49b..d6e29daced63 100644
> --- a/nbd/client.c
> +++ 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...
> 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");
> +nbd_send_opt_abort(ioc);
> +return -1;
> +}
> local_desc = g_malloc(len + 1);
> if (nbd_read(ioc, local_desc, len,
