| Message ID | 20191114024635.11363-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Better NBD string length handling | expand |
On 11/13/19 8:46 PM, Eric Blake wrote: > As long as we limit NBD names to 256 bytes (the bare minimum permitted > by the standard), stack-allocation works for parsing a name received > from the client. But as mentioned in a comment, we eventually want to > permit up to the 4k maximum of the NBD standard, which is too large > for stack allocation; so switch everything in the server to use heap > allocation. For now, there is no change in actually supported name > length. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/block/nbd.h | 10 +++++----- > nbd/server.c | 25 +++++++++++++++---------- > 2 files changed, 20 insertions(+), 15 deletions(-) > @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client) > static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, > Error **errp) > { > - char name[NBD_MAX_NAME_SIZE + 1]; > + g_autofree char *name; This needs to be: g_autofree char *name = NULL;
On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote: > As long as we limit NBD names to 256 bytes (the bare minimum permitted > by the standard), stack-allocation works for parsing a name received > from the client. But as mentioned in a comment, we eventually want to > permit up to the 4k maximum of the NBD standard, which is too large > for stack allocation; so switch everything in the server to use heap > allocation. For now, there is no change in actually supported name > length. I am just curios, why is this so? I know that kernel uses 8K stacks due to historical limitation of 1:1 physical memory mapping which creates fragmentation, but in the userspace stacks shouldn't really be limited and grow on demand. Some gcc security option limits this? > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/block/nbd.h | 10 +++++----- > nbd/server.c | 25 +++++++++++++++---------- > 2 files changed, 20 insertions(+), 15 deletions(-) > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index 316fd705a9e4..c306423dc85c 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -226,11 +226,11 @@ enum { > /* Maximum size of a single READ/WRITE data buffer */ > #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024) > > -/* Maximum size of an export name. The NBD spec requires 256 and > - * suggests that servers support up to 4096, but we stick to only the > - * required size so that we can stack-allocate the names, and because > - * going larger would require an audit of more code to make sure we > - * aren't overflowing some other buffer. */ > +/* > + * Maximum size of an export name. The NBD spec requires a minimum of > + * 256 and recommends that servers support up to 4096; all users use > + * malloc so we can bump this constant without worry. > + */ > #define NBD_MAX_NAME_SIZE 256 > > /* Two types of reply structures */ > diff --git a/nbd/server.c b/nbd/server.c > index d8d1e6245532..c63b76b22735 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -324,18 +324,20 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp) > * uint32_t len (<= NBD_MAX_NAME_SIZE) > * len bytes string (not 0-terminated) > * > - * @name should be enough to store NBD_MAX_NAME_SIZE+1. > + * On success, @name will be allocated. > * If @length is non-null, it will be set to the actual string length. > * > * Return -errno on I/O error, 0 if option was completely handled by > * sending a reply about inconsistent lengths, or 1 on success. > */ > -static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length, > +static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length, > Error **errp) > { > int ret; > uint32_t len; > + g_autofree char *local_name = NULL; > > + *name = NULL; > ret = nbd_opt_read(client, &len, sizeof(len), errp); > if (ret <= 0) { > return ret; > @@ -347,15 +349,17 @@ static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length, > "Invalid name length: %" PRIu32, len); > } > > - ret = nbd_opt_read(client, name, len, errp); > + local_name = g_malloc(len + 1); > + ret = nbd_opt_read(client, local_name, len, errp); > if (ret <= 0) { > return ret; > } > - name[len] = '\0'; > + local_name[len] = '\0'; > > if (length) { > *length = len; > } > + *name = g_steal_pointer(&local_name); > > return 1; > } > @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client) > static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, > Error **errp) > { > - char name[NBD_MAX_NAME_SIZE + 1]; > + g_autofree char *name; That is what patchew complained about I think. Isn't it wonderful how g_autofree fixes one issue and introduces another. I mean 'name' isn't really used here prior to allocation according to plain C, but due to g_autofree, it can be now on any error path. Nothing against g_autofree though, just noting this. > char buf[NBD_REPLY_EXPORT_NAME_SIZE] = ""; > size_t len; > int ret; > @@ -441,10 +445,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, > [10 .. 133] reserved (0) [unless no_zeroes] > */ > trace_nbd_negotiate_handle_export_name(); > - if (client->optlen >= sizeof(name)) { > + if (client->optlen > NBD_MAX_NAME_SIZE) { > error_setg(errp, "Bad length received"); > return -EINVAL; > } > + name = g_malloc(client->optlen + 1); > if (nbd_read(client->ioc, name, client->optlen, "export name", errp) < 0) { > return -EIO; > } > @@ -533,7 +538,7 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp) > static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) > { > int rc; > - char name[NBD_MAX_NAME_SIZE + 1]; > + g_autofree char *name = NULL; > NBDExport *exp; > uint16_t requests; > uint16_t request; > @@ -551,7 +556,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) > 2 bytes: N, number of requests (can be 0) > N * 2 bytes: N requests > */ > - rc = nbd_opt_read_name(client, name, &namelen, errp); > + rc = nbd_opt_read_name(client, &name, &namelen, errp); > if (rc <= 0) { > return rc; > } > @@ -957,7 +962,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > NBDExportMetaContexts *meta, Error **errp) > { > int ret; > - char export_name[NBD_MAX_NAME_SIZE + 1]; > + g_autofree char *export_name = NULL; > NBDExportMetaContexts local_meta; > uint32_t nb_queries; > int i; > @@ -976,7 +981,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client, > > memset(meta, 0, sizeof(*meta)); > > - ret = nbd_opt_read_name(client, export_name, NULL, errp); > + ret = nbd_opt_read_name(client, &export_name, NULL, errp); > if (ret <= 0) { > return ret; > } Looks correct, but I might have missed something. Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On 11/14/19 4:04 AM, Maxim Levitsky wrote: > On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote: >> As long as we limit NBD names to 256 bytes (the bare minimum permitted >> by the standard), stack-allocation works for parsing a name received >> from the client. But as mentioned in a comment, we eventually want to >> permit up to the 4k maximum of the NBD standard, which is too large >> for stack allocation; so switch everything in the server to use heap >> allocation. For now, there is no change in actually supported name >> length. > > I am just curios, why is this so? > I know that kernel uses 8K stacks due to historical limitation > of 1:1 physical memory mapping which creates fragmentation, > but in the userspace stacks shouldn't really be limited and grow on demand. Actually, 4k rather than 8k stack overflow guard pages are typical on some OS. The problem with stack-allocating anything larger than the guard page size is that you can end up overshooting the guard page, and then the OS is unable to catch stack overflow in the normal manner of sending SIGSEGV. Also, when using coroutines, it is very common to have limited stack size in the first place, where large stack allocations can run into issues. So in general, it's a good rule of thumb to never stack-allocate something if it can be larger than 4k. > Some gcc security option limits this? Not by default, but you can compile with -Wframe-larger-than=4096 (or even smaller) to catch instances where stack allocation is likely to run into trouble. >> @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client) >> static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, >> Error **errp) >> { >> - char name[NBD_MAX_NAME_SIZE + 1]; >> + g_autofree char *name; > > That is what patchew complained about I think. Yes, and I've already fixed the missing initializer. > > Isn't it wonderful how g_autofree fixes one issue > and introduces another. I mean 'name' isn't really > used here prior to allocation according to plain C, > but due to g_autofree, it can be now on any error > path. Nothing against g_autofree though, just noting this. Yes, and our documentation for g_auto* reminds that all such variables with automatic cleanup must have an initializer or be set prior to any exit path. I think I see why I didn't catch it beforehand - I'm compiling with --enable-debug, which passes CFLAGS=-g, while the compiler warning occurs when -O2 is in effect; but it is rather annoying that gcc doesn't catch the bug when not optimizing. > > Looks correct, but I might have missed something. > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > Thanks, and assuming that's with my initializer fix squashed in. > Best regards, > Maxim Levitsky > >
14.11.2019 5:46, Eric Blake wrote: > As long as we limit NBD names to 256 bytes (the bare minimum permitted > by the standard), stack-allocation works for parsing a name received > from the client. But as mentioned in a comment, we eventually want to > permit up to the 4k maximum of the NBD standard, which is too large > for stack allocation; so switch everything in the server to use heap > allocation. For now, there is no change in actually supported name > length. > > Signed-off-by: Eric Blake <eblake@redhat.com> with mentioned fix: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> PS: Great, how using g_autofree simplifies reviewing! I don't need to check that something is leaked at some return point.
On Thu, 2019-11-14 at 07:33 -0600, Eric Blake wrote: > On 11/14/19 4:04 AM, Maxim Levitsky wrote: > > On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote: > > > As long as we limit NBD names to 256 bytes (the bare minimum permitted > > > by the standard), stack-allocation works for parsing a name received > > > from the client. But as mentioned in a comment, we eventually want to > > > permit up to the 4k maximum of the NBD standard, which is too large > > > for stack allocation; so switch everything in the server to use heap > > > allocation. For now, there is no change in actually supported name > > > length. > > > > I am just curios, why is this so? > > I know that kernel uses 8K stacks due to historical limitation > > of 1:1 physical memory mapping which creates fragmentation, > > but in the userspace stacks shouldn't really be limited and grow on demand. > > Actually, 4k rather than 8k stack overflow guard pages are typical on > some OS. I was talking about the kernel stacks. These are limited to 8K with no growing and it is a pain point there. Userspace stacks on the other hand should be able to grow to an reasonable size. > The problem with stack-allocating anything larger than the > guard page size is that you can end up overshooting the guard page, and > then the OS is unable to catch stack overflow in the normal manner of > sending SIGSEGV. Also, when using coroutines, it is very common to have > limited stack size in the first place, where large stack allocations can > run into issues. So in general, it's a good rule of thumb to never > stack-allocate something if it can be larger than 4k. Doh! I know how the guard pages work, but never thought about them in this way. I guess I don't after all. Thanks for the explanation! > > > Some gcc security option limits this? > > Not by default, but you can compile with -Wframe-larger-than=4096 (or > even smaller) to catch instances where stack allocation is likely to run > into trouble. > > > > > @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client) > > > static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, > > > Error **errp) > > > { > > > - char name[NBD_MAX_NAME_SIZE + 1]; > > > + g_autofree char *name; > > > > That is what patchew complained about I think. > > Yes, and I've already fixed the missing initializer. > > > > > Isn't it wonderful how g_autofree fixes one issue > > and introduces another. I mean 'name' isn't really > > used here prior to allocation according to plain C, > > but due to g_autofree, it can be now on any error > > path. Nothing against g_autofree though, just noting this. > > Yes, and our documentation for g_auto* reminds that all such variables > with automatic cleanup must have an initializer or be set prior to any > exit path. I think I see why I didn't catch it beforehand - I'm > compiling with --enable-debug, which passes CFLAGS=-g, while the > compiler warning occurs when -O2 is in effect; but it is rather annoying > that gcc doesn't catch the bug when not optimizing. > > > > > Looks correct, but I might have missed something. > > > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > Thanks, and assuming that's with my initializer fix squashed in. Of course. Best regards, Maxim Levitsky
diff --git a/include/block/nbd.h b/include/block/nbd.h index 316fd705a9e4..c306423dc85c 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -226,11 +226,11 @@ enum { /* Maximum size of a single READ/WRITE data buffer */ #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024) -/* Maximum size of an export name. The NBD spec requires 256 and - * suggests that servers support up to 4096, but we stick to only the - * required size so that we can stack-allocate the names, and because - * going larger would require an audit of more code to make sure we - * aren't overflowing some other buffer. */ +/* + * Maximum size of an export name. The NBD spec requires a minimum of + * 256 and recommends that servers support up to 4096; all users use + * malloc so we can bump this constant without worry. + */ #define NBD_MAX_NAME_SIZE 256 /* Two types of reply structures */ diff --git a/nbd/server.c b/nbd/server.c index d8d1e6245532..c63b76b22735 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -324,18 +324,20 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp) * uint32_t len (<= NBD_MAX_NAME_SIZE) * len bytes string (not 0-terminated) * - * @name should be enough to store NBD_MAX_NAME_SIZE+1. + * On success, @name will be allocated. * If @length is non-null, it will be set to the actual string length. * * Return -errno on I/O error, 0 if option was completely handled by * sending a reply about inconsistent lengths, or 1 on success. */ -static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length, +static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length, Error **errp) { int ret; uint32_t len; + g_autofree char *local_name = NULL; + *name = NULL; ret = nbd_opt_read(client, &len, sizeof(len), errp); if (ret <= 0) { return ret; @@ -347,15 +349,17 @@ static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length, "Invalid name length: %" PRIu32, len); } - ret = nbd_opt_read(client, name, len, errp); + local_name = g_malloc(len + 1); + ret = nbd_opt_read(client, local_name, len, errp); if (ret <= 0) { return ret; } - name[len] = '\0'; + local_name[len] = '\0'; if (length) { *length = len; } + *name = g_steal_pointer(&local_name); return 1; } @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client) static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, Error **errp) { - char name[NBD_MAX_NAME_SIZE + 1]; + g_autofree char *name; char buf[NBD_REPLY_EXPORT_NAME_SIZE] = ""; size_t len; int ret; @@ -441,10 +445,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, [10 .. 133] reserved (0) [unless no_zeroes] */ trace_nbd_negotiate_handle_export_name(); - if (client->optlen >= sizeof(name)) { + if (client->optlen > NBD_MAX_NAME_SIZE) { error_setg(errp, "Bad length received"); return -EINVAL; } + name = g_malloc(client->optlen + 1); if (nbd_read(client->ioc, name, client->optlen, "export name", errp) < 0) { return -EIO; } @@ -533,7 +538,7 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp) static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) { int rc; - char name[NBD_MAX_NAME_SIZE + 1]; + g_autofree char *name = NULL; NBDExport *exp; uint16_t requests; uint16_t request; @@ -551,7 +556,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) 2 bytes: N, number of requests (can be 0) N * 2 bytes: N requests */ - rc = nbd_opt_read_name(client, name, &namelen, errp); + rc = nbd_opt_read_name(client, &name, &namelen, errp); if (rc <= 0) { return rc; } @@ -957,7 +962,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client, NBDExportMetaContexts *meta, Error **errp) { int ret; - char export_name[NBD_MAX_NAME_SIZE + 1]; + g_autofree char *export_name = NULL; NBDExportMetaContexts local_meta; uint32_t nb_queries; int i; @@ -976,7 +981,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client, memset(meta, 0, sizeof(*meta)); - ret = nbd_opt_read_name(client, export_name, NULL, errp); + ret = nbd_opt_read_name(client, &export_name, NULL, errp); if (ret <= 0) { return ret; }
As long as we limit NBD names to 256 bytes (the bare minimum permitted by the standard), stack-allocation works for parsing a name received from the client. But as mentioned in a comment, we eventually want to permit up to the 4k maximum of the NBD standard, which is too large for stack allocation; so switch everything in the server to use heap allocation. For now, there is no change in actually supported name length. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/block/nbd.h | 10 +++++----- nbd/server.c | 25 +++++++++++++++---------- 2 files changed, 20 insertions(+), 15 deletions(-)