| Message ID | 1460153158-21612-7-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8 Apr 2016, at 23:05, Eric Blake <eblake@redhat.com> wrote: > Declare a constant and use that when determining if an export > name fits within the constraints we are willing to support. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/block/nbd.h | 2 ++ > nbd/client.c | 2 +- > nbd/server.c | 4 ++-- > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index b86a976..3f047bf 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -76,6 +76,8 @@ enum { > > /* Maximum size of a single READ/WRITE data buffer */ > #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024) > +/* Maximum size of an export name */ > +#define NBD_MAX_NAME_SIZE 255 Given the standard is either likely to or does (can't remember whether that patch is merged) document the maximum supported export length as 4096, why not change this to 4096? Otherwise: Reviewed-by: Alex Bligh <alex@alex.org.uk> > > ssize_t nbd_wr_syncv(QIOChannel *ioc, > struct iovec *iov, > diff --git a/nbd/client.c b/nbd/client.c > index c834587..00f9244 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -210,7 +210,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp) > error_setg(errp, "incorrect option name length"); > return -1; > } > - if (namelen > 255) { > + if (namelen > NBD_MAX_NAME_SIZE) { > error_setg(errp, "export name length too long %" PRIu32, namelen); > return -1; > } > diff --git a/nbd/server.c b/nbd/server.c > index a10294e..5414c49 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -285,13 +285,13 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length) > static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length) > { > int rc = -EINVAL; > - char name[256]; > + char name[NBD_MAX_NAME_SIZE + 1]; > > /* Client sends: > [20 .. xx] export name (length bytes) > */ > TRACE("Checking length"); > - if (length > 255) { > + if (length >= sizeof(name)) { > LOG("Bad length received"); > goto fail; > } > -- > 2.5.5 > >
On 04/09/2016 04:35 AM, Alex Bligh wrote: > > On 8 Apr 2016, at 23:05, Eric Blake <eblake@redhat.com> wrote: > >> Declare a constant and use that when determining if an export >> name fits within the constraints we are willing to support. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> +/* Maximum size of an export name */ >> +#define NBD_MAX_NAME_SIZE 255 > > Given the standard is either likely to or does (can't > remember whether that patch is merged) document the > maximum supported export length as 4096, why not change > this to 4096? I think I'd rather change the limit in a separate patch, auditing to make sure that it works. The current NBD Protocol states: Where this document refers to a string, then unless otherwise stated, that string is a sequence of UTF-8 code points, which is not NUL terminated, MUST NOT contain NUL characters, SHOULD be no longer than 256 bytes and MUST be no longer than 4096 bytes. So I agree that 255 is too small - a client sending 256 bytes and being rejected by the server is a bug in the server, while a client sending 4096 bytes and being rejected by the server is not a protocol violation, just poorer quality of implementation. Also, I think that the server should gracefully reject the client for 4096 (as in a nice NBD_ERR_REP_POLICY in response to NBD_OPT_INFO, stating that "your request was valid by protocol, but not something I'm willing to handle"), rather than abruptly disconnecting; while dealing with a client that sends a 100M name is more likely to be a Denial-of-service attack where an abrupt disconnect is nicer than wasting time reading to the end of the client's message. On the other hand, 4096 bytes is big enough that you can't safely stack allocate (we are trying to get qemu to the point where it can be compiled with gcc's options to warn on any function that requires more than 4096 bytes for the entire function, as that is the largest safe amount you can have before you can run into stack overflow turning what is supposed to be SIGSEGV into a hard kill on Windows, if you get unlucky enough to skip over the guard page). Also, qemu has smaller limits in other places (for example, no more than 1024 bytes for the name of a qcow2 backing file), so it does no good to make qemu support 4096 bytes in NBD if it can't pass that on to the rest of qemu. At any rate, I should probably stick this above explanation in the commit message (or else do the audit, and merge it into this patch after all, even if I pick a different limit than 4096). > > Otherwise: > > Reviewed-by: Alex Bligh <alex@alex.org.uk> >
diff --git a/include/block/nbd.h b/include/block/nbd.h index b86a976..3f047bf 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -76,6 +76,8 @@ enum { /* Maximum size of a single READ/WRITE data buffer */ #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024) +/* Maximum size of an export name */ +#define NBD_MAX_NAME_SIZE 255 ssize_t nbd_wr_syncv(QIOChannel *ioc, struct iovec *iov, diff --git a/nbd/client.c b/nbd/client.c index c834587..00f9244 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -210,7 +210,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp) error_setg(errp, "incorrect option name length"); return -1; } - if (namelen > 255) { + if (namelen > NBD_MAX_NAME_SIZE) { error_setg(errp, "export name length too long %" PRIu32, namelen); return -1; } diff --git a/nbd/server.c b/nbd/server.c index a10294e..5414c49 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -285,13 +285,13 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length) static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length) { int rc = -EINVAL; - char name[256]; + char name[NBD_MAX_NAME_SIZE + 1]; /* Client sends: [20 .. xx] export name (length bytes) */ TRACE("Checking length"); - if (length > 255) { + if (length >= sizeof(name)) { LOG("Bad length received"); goto fail; }
Declare a constant and use that when determining if an export name fits within the constraints we are willing to support. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/block/nbd.h | 2 ++ nbd/client.c | 2 +- nbd/server.c | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-)