Hi On Sun, May 19, 2019 at 10:55 AM P J P <[email protected]> wrote: > > From: Prasad J Pandit <[email protected]> > > Qemu guest agent while executing user commands does not seem to > check length of argument list and/or environment variables passed. > It may lead to integer overflow or infinite loop issues. Add check > to avoid it.
Are you intentionally not telling where these overflow or loop happen? Isn't the kernel already giving an error if given too much environment/arguments on exec? > > Reported-by: Niu Guoxiang <[email protected]> > Signed-off-by: Prasad J Pandit <[email protected]> > --- > qga/commands-posix.c | 18 ++++++++++++++++++ > qga/commands-win32.c | 13 +++++++++++++ > qga/commands.c | 8 ++++++-- > qga/guest-agent-core.h | 2 ++ > 4 files changed, 39 insertions(+), 2 deletions(-) > > Update v2: add helper function ga_get_arg_max() > -> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06360.html > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 7ee6a33cce..e0455722e0 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -60,6 +60,24 @@ extern char **environ; > #endif > #endif > > +size_t ga_get_arg_max(void) > +{ > + /* Since kernel 2.6.23, most architectures support argument size limit > + * derived from the soft RLIMIT_STACK resource limit (see getrlimit(2)). > + * For these architectures, the total size is limited to 1/4 of the > + * allowed stack size. (see execve(2)) > + * > + * struct rlimit r; > + * > + * getrlimit(RLIMIT_STACK, &r); > + * ARG_MAX = r.rlim_cur / 4; > + * > + * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the > + * memory used to hold command line arguments and environment variables. > + */ > + return sysconf(_SC_ARG_MAX); > +} > + > static void ga_wait_child(pid_t pid, int *status, Error **errp) > { > pid_t rpid; > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 6b67f16faf..47bbddd74a 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -92,6 +92,19 @@ static OpenFlags guest_file_open_modes[] = { > g_free(suffix); \ > } while (0) > > +size_t ga_get_arg_max(void) > +{ > + /* Win32 environment has different values for the ARG_MAX constant. > + * We'll go with the maximum here. > + * > + * https://devblogs.microsoft.com/oldnewthing/?p=41553 > + * > + * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the > + * memory used to hold command line arguments and environment variables. > + */ > + return 32767; > +} > + > static OpenFlags *find_open_flag(const char *mode_str) > { > int mode; > diff --git a/qga/commands.c b/qga/commands.c > index 0c7d1385c2..425a4c405f 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -231,17 +231,21 @@ static char **guest_exec_get_args(const strList *entry, > bool log) > int count = 1, i = 0; /* reserve for NULL terminator */ > char **args; > char *str; /* for logging array of arguments */ > - size_t str_size = 1; > + size_t str_size = 1, arg_max; > > + arg_max = ga_get_arg_max(); > for (it = entry; it != NULL; it = it->next) { > count++; > str_size += 1 + strlen(it->value); > + if (str_size >= arg_max || count >= arg_max / 2) { > + break; This seems to silently drop remaining arguments, which is probably not what you want. > + } > } > > str = g_malloc(str_size); > *str = 0; > args = g_malloc(count * sizeof(char *)); > - for (it = entry; it != NULL; it = it->next) { > + for (it = entry; it != NULL && i < count; it = it->next) { > args[i++] = it->value; > pstrcat(str, str_size, it->value); > if (it->next) { > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h > index 60eae16f27..300ff7e482 100644 > --- a/qga/guest-agent-core.h > +++ b/qga/guest-agent-core.h > @@ -46,6 +46,8 @@ const char *ga_fsfreeze_hook(GAState *s); > int64_t ga_get_fd_handle(GAState *s, Error **errp); > int ga_parse_whence(GuestFileWhence *whence, Error **errp); > > +size_t ga_get_arg_max(void); > + > #ifndef _WIN32 > void reopen_fd_to_null(int fd); > #endif > -- > 2.20.1 > > -- Marc-André Lureau
