[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] nbd: Don't let client send oversize strings
From: |
Eric Blake |
Subject: |
Re: [PATCH] nbd: Don't let client send oversize strings |
Date: |
Wed, 9 Oct 2019 10:30:19 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
On 9/29/19 1:49 PM, Maxim Levitsky wrote:
On Fri, 2019-09-27 at 23:13 -0500, Eric Blake wrote:
Qemu as server currently won't accept export names larger than 256
bytes, so most uses of qemu as client have no reason to get anywhere
near the NBD spec maximum of a 4k limit per string. However, we
didn't actually have any code that prevented the client from violating
the protocol, which, while useful for testing corner-case server
reactions, is probably not ideal.
Signed-off-by: Eric Blake <address@hidden>
---
include/block/nbd.h | 1 +
nbd/client.c | 8 ++++++++
2 files changed, 9 insertions(+)
+++ b/nbd/client.c
@@ -648,6 +648,10 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t
opt,
if (query) {
query_len = strlen(query);
data_len += sizeof(query_len) + query_len;
+ if (query_len > NBD_MAX_STRING_SIZE) {
+ error_setg(errp, "x_dirty_bitmap query too long to send to
server");
Is there a way not to do this here? I don't know nbd well to be honest,
and it looks like this code currently is only called for x_dirty_bitmap but
there could be more cases in the future.
I could make this an assert, and fix the callers to pass in valid
lengths (callers pass in either "base:allocation" which fits, or a
user-supplied x_dirty_bitmap, so validating at the point that hack is
assigned is reasoanble).
nbd_negotiate_simple_meta_context which seems to be the caller of this, already
mentions
a 'hack' about this :-(
Of course if you think that this is not worth the time, you can leave this as
is.
+ return -1;
+ }
} else {
assert(opt == NBD_OPT_LIST_META_CONTEXT);
}
@@ -1010,6 +1014,10 @@ int nbd_receive_negotiate(AioContext *aio_context,
QIOChannel *ioc,
bool base_allocation = info->base_allocation;
assert(info->name);
+ if (strlen(info->name) > NBD_MAX_STRING_SIZE) {
+ error_setg(errp, "name too long to send to server");
Maybe 'export name'?
Sure.
+ return -EINVAL;
+ }
trace_nbd_receive_negotiate_name(info->name);
result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
Why not to do the export name check when info->name is set, that is in
nbd_client_connect?
I'll spin up a v2.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH] nbd: Don't let client send oversize strings,
Eric Blake <=