| Message ID | 2931c2a6-414b-4193-bd6b-38c38c858677@HUB2.rwth-ad.de |
|---|---|
| State | New |
| Headers | show |
On 15 August 2015 at 19:26, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > qemu currently limits the space for the evironment and arguments to > 32 * PAGE_SIZE. Linux limits the argument space to 1/4 of the stack size. > A program trying to detect this with a getrlimit(RLIMIT_STACK) syscall > will typically get a much larger limit than qemus current 128kB. > > The current limit causes "Argument list too long" errors. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> Thanks for this bug fix; it definitely seems like a good idea. I have a few review comments below. > --- > linux-user/elfload.c | 29 ++++++++++++++++++----------- > linux-user/linuxload.c | 7 ++++--- > linux-user/qemu.h | 11 ++--------- > linux-user/syscall.c | 4 ++++ > 4 files changed, 28 insertions(+), 23 deletions(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index 1788368..be8f4d6 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -1365,11 +1365,13 @@ static bool elf_check_ehdr(struct elfhdr *ehdr) > * to be put directly into the top of new user memory. > * > */ > -static abi_ulong copy_elf_strings(int argc,char ** argv, void **page, > - abi_ulong p) > +static abi_ulong copy_elf_strings(int argc,char ** argv, This should have a space after the 'argc,'. (If you run scripts/checkpatch.pl you'll find it catches this and other minor style errors.) > + struct linux_binprm *bprm) > { > char *tmp, *tmp1, *pag = NULL; > int len, offset = 0; > + void **page = bprm->page; > + abi_ulong p = bprm->p; > > if (!p) { > return 0; /* bullet-proofing */ > @@ -1383,8 +1385,13 @@ static abi_ulong copy_elf_strings(int argc,char ** argv, void **page, > tmp1 = tmp; > while (*tmp++); > len = tmp - tmp1; > - if (p < len) { /* this shouldn't happen - 128kB */ > - return 0; > + if (p < len) { Since this looks almost but not quite like a standard reallocate-larger, a comment here would be helpful I think: /* Reallocate the page array to add extra zero entries at the start */ > + bprm->page = (void**)calloc(bprm->n_arg_pages + 32, sizeof(void*)); Prefer bprm->page = g_new0(void *, bprm->n_arg_pages + 32); > + memcpy(&bprm->page[32], page, sizeof(void*) * bprm->n_arg_pages); > + free(page); g_free(page); > + page = bprm->page; > + bprm->n_arg_pages += 32; > + p += 32 * TARGET_PAGE_SIZE; I think we have enough repetitions of '32' here to merit a #define. But having said all that, I wonder if it would be better to precalculate how big a page array we need and just do the allocation once, rather than having this complicated code to handle a reallocate-and-fix-up-everything. In particular this is basically just adding string lengths for filename, argv and envp together. load_flt_binary() already wants that information, so it might be better to have loader_exec() calculate this and fill in new bprm->argv_strlen and bprm->envp_strlen values for the callees to use. > } > while (len) { > --p; --tmp; --len; > @@ -1423,8 +1430,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, > /* Create enough stack to hold everything. If we don't use > it for args, we'll use it for something else. */ > size = guest_stack_size; > - if (size < MAX_ARG_PAGES*TARGET_PAGE_SIZE) { > - size = MAX_ARG_PAGES*TARGET_PAGE_SIZE; > + if (size < bprm->n_arg_pages * TARGET_PAGE_SIZE) { > + size = bprm->n_arg_pages * TARGET_PAGE_SIZE; > } > guard = TARGET_PAGE_SIZE; > if (guard < qemu_real_host_page_size) { > @@ -1442,10 +1449,10 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, > target_mprotect(error, guard, PROT_NONE); > > info->stack_limit = error + guard; > - stack_base = info->stack_limit + size - MAX_ARG_PAGES*TARGET_PAGE_SIZE; > + stack_base = info->stack_limit + size - bprm->n_arg_pages * TARGET_PAGE_SIZE; > p += stack_base; > > - for (i = 0 ; i < MAX_ARG_PAGES ; i++) { > + for (i = 0; i < bprm->n_arg_pages; i++) { > if (bprm->page[i]) { > info->rss++; > /* FIXME - check return value of memcpy_to_target() for failure */ > @@ -2211,9 +2218,9 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info) > when we load the interpreter. */ > elf_ex = *(struct elfhdr *)bprm->buf; > > - bprm->p = copy_elf_strings(1, &bprm->filename, bprm->page, bprm->p); > - bprm->p = copy_elf_strings(bprm->envc,bprm->envp,bprm->page,bprm->p); > - bprm->p = copy_elf_strings(bprm->argc,bprm->argv,bprm->page,bprm->p); > + bprm->p = copy_elf_strings(1, &bprm->filename, bprm); > + bprm->p = copy_elf_strings(bprm->envc, bprm->envp, bprm); > + bprm->p = copy_elf_strings(bprm->argc, bprm->argv, bprm); > if (!bprm->p) { > fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG)); > exit(-1); > diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c > index 506e837..59943ea 100644 > --- a/linux-user/linuxload.c > +++ b/linux-user/linuxload.c > @@ -137,8 +137,9 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp, > int retval; > int i; > > - bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int); > - memset(bprm->page, 0, sizeof(bprm->page)); > + bprm->n_arg_pages = 32; > + bprm->p = bprm->n_arg_pages * TARGET_PAGE_SIZE - sizeof(unsigned int); > + bprm->page = (void**)calloc(bprm->n_arg_pages, sizeof(void*)); Rather than calloc, prefer bprm->page = g_new0(void *, bprm->n_arg_pages); > bprm->fd = fdexec; > bprm->filename = (char *)filename; > bprm->argc = count(argv); > @@ -173,7 +174,7 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp, > } > > /* Something went wrong, return the inode and free the argument pages*/ > - for (i=0 ; i<MAX_ARG_PAGES ; i++) { > + for (i = 0; i < bprm->n_arg_pages; i++) { > g_free(bprm->page[i]); > } You should g_free() bprm->page too here now, since we now allocate it. > return(retval); > diff --git a/linux-user/qemu.h b/linux-user/qemu.h > index 8012cc2..cf4aa73 100644 > --- a/linux-user/qemu.h > +++ b/linux-user/qemu.h > @@ -144,14 +144,6 @@ void stop_all_tasks(void); > extern const char *qemu_uname_release; > extern unsigned long mmap_min_addr; > > -/* ??? See if we can avoid exposing so much of the loader internals. */ > -/* > - * MAX_ARG_PAGES defines the number of pages allocated for arguments > - * and envelope for the new program. 32 should suffice, this gives > - * a maximum env+arg of 128kB w/4KB pages! > - */ > -#define MAX_ARG_PAGES 33 > - > /* Read a good amount of data initially, to hopefully get all the > program headers loaded. */ > #define BPRM_BUF_SIZE 1024 > @@ -162,7 +154,8 @@ extern unsigned long mmap_min_addr; > */ > struct linux_binprm { > char buf[BPRM_BUF_SIZE] __attribute__((aligned)); > - void *page[MAX_ARG_PAGES]; > + void **page; > + int n_arg_pages; > abi_ulong p; > int fd; > int e_uid, e_gid; > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index f62c698..6fa5fb4 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -5799,12 +5799,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > } > *q = NULL; > > +/* This check is bogus. MAX_ARG_PAGES is a thing of the past. Trust the host > +execve to set errno appropriately */ > +#if 0 > /* This case will not be caught by the host's execve() if its > page size is bigger than the target's. */ > if (total_size > MAX_ARG_PAGES * TARGET_PAGE_SIZE) { > ret = -TARGET_E2BIG; > goto execve_end; > } > +#endif You should just delete this code now it's not required; we don't need to leave #if-0'd out obsolete code in the tree. > if (!(p = lock_user_string(arg1))) > goto execve_efault; > ret = get_errno(execve(p, argp, envp)); > -- > 2.1.4 thanks -- PMM
On Wednesday 19 August 2015 22:57:53 you wrote: [...] > > I think we have enough repetitions of '32' here to merit a #define. > > But having said all that, I wonder if it would be better to > precalculate how big a page array we need and just do the > allocation once, rather than having this complicated code to > handle a reallocate-and-fix-up-everything. In particular this > is basically just adding string lengths for filename, argv > and envp together. load_flt_binary() already wants that information, > so it might be better to have loader_exec() calculate this > and fill in new bprm->argv_strlen and bprm->envp_strlen values > for the callees to use. I have completely reworked the patch. There is no longer any need for the page array, the environment gets directly copied to the target stack (although it uses a scratch buffer, to avoid frequent calls to the locking memcpy_to_target). Kind regards, Stefan
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 1788368..be8f4d6 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -1365,11 +1365,13 @@ static bool elf_check_ehdr(struct elfhdr *ehdr) * to be put directly into the top of new user memory. * */ -static abi_ulong copy_elf_strings(int argc,char ** argv, void **page, - abi_ulong p) +static abi_ulong copy_elf_strings(int argc,char ** argv, + struct linux_binprm *bprm) { char *tmp, *tmp1, *pag = NULL; int len, offset = 0; + void **page = bprm->page; + abi_ulong p = bprm->p; if (!p) { return 0; /* bullet-proofing */ @@ -1383,8 +1385,13 @@ static abi_ulong copy_elf_strings(int argc,char ** argv, void **page, tmp1 = tmp; while (*tmp++); len = tmp - tmp1; - if (p < len) { /* this shouldn't happen - 128kB */ - return 0; + if (p < len) { + bprm->page = (void**)calloc(bprm->n_arg_pages + 32, sizeof(void*)); + memcpy(&bprm->page[32], page, sizeof(void*) * bprm->n_arg_pages); + free(page); + page = bprm->page; + bprm->n_arg_pages += 32; + p += 32 * TARGET_PAGE_SIZE; } while (len) { --p; --tmp; --len; @@ -1423,8 +1430,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, /* Create enough stack to hold everything. If we don't use it for args, we'll use it for something else. */ size = guest_stack_size; - if (size < MAX_ARG_PAGES*TARGET_PAGE_SIZE) { - size = MAX_ARG_PAGES*TARGET_PAGE_SIZE; + if (size < bprm->n_arg_pages * TARGET_PAGE_SIZE) { + size = bprm->n_arg_pages * TARGET_PAGE_SIZE; } guard = TARGET_PAGE_SIZE; if (guard < qemu_real_host_page_size) { @@ -1442,10 +1449,10 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, target_mprotect(error, guard, PROT_NONE); info->stack_limit = error + guard; - stack_base = info->stack_limit + size - MAX_ARG_PAGES*TARGET_PAGE_SIZE; + stack_base = info->stack_limit + size - bprm->n_arg_pages * TARGET_PAGE_SIZE; p += stack_base; - for (i = 0 ; i < MAX_ARG_PAGES ; i++) { + for (i = 0; i < bprm->n_arg_pages; i++) { if (bprm->page[i]) { info->rss++; /* FIXME - check return value of memcpy_to_target() for failure */ @@ -2211,9 +2218,9 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info) when we load the interpreter. */ elf_ex = *(struct elfhdr *)bprm->buf; - bprm->p = copy_elf_strings(1, &bprm->filename, bprm->page, bprm->p); - bprm->p = copy_elf_strings(bprm->envc,bprm->envp,bprm->page,bprm->p); - bprm->p = copy_elf_strings(bprm->argc,bprm->argv,bprm->page,bprm->p); + bprm->p = copy_elf_strings(1, &bprm->filename, bprm); + bprm->p = copy_elf_strings(bprm->envc, bprm->envp, bprm); + bprm->p = copy_elf_strings(bprm->argc, bprm->argv, bprm); if (!bprm->p) { fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG)); exit(-1); diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c index 506e837..59943ea 100644 --- a/linux-user/linuxload.c +++ b/linux-user/linuxload.c @@ -137,8 +137,9 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp, int retval; int i; - bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int); - memset(bprm->page, 0, sizeof(bprm->page)); + bprm->n_arg_pages = 32; + bprm->p = bprm->n_arg_pages * TARGET_PAGE_SIZE - sizeof(unsigned int); + bprm->page = (void**)calloc(bprm->n_arg_pages, sizeof(void*)); bprm->fd = fdexec; bprm->filename = (char *)filename; bprm->argc = count(argv); @@ -173,7 +174,7 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp, } /* Something went wrong, return the inode and free the argument pages*/ - for (i=0 ; i<MAX_ARG_PAGES ; i++) { + for (i = 0; i < bprm->n_arg_pages; i++) { g_free(bprm->page[i]); } return(retval); diff --git a/linux-user/qemu.h b/linux-user/qemu.h index 8012cc2..cf4aa73 100644 --- a/linux-user/qemu.h +++ b/linux-user/qemu.h @@ -144,14 +144,6 @@ void stop_all_tasks(void); extern const char *qemu_uname_release; extern unsigned long mmap_min_addr; -/* ??? See if we can avoid exposing so much of the loader internals. */ -/* - * MAX_ARG_PAGES defines the number of pages allocated for arguments - * and envelope for the new program. 32 should suffice, this gives - * a maximum env+arg of 128kB w/4KB pages! - */ -#define MAX_ARG_PAGES 33 - /* Read a good amount of data initially, to hopefully get all the program headers loaded. */ #define BPRM_BUF_SIZE 1024 @@ -162,7 +154,8 @@ extern unsigned long mmap_min_addr; */ struct linux_binprm { char buf[BPRM_BUF_SIZE] __attribute__((aligned)); - void *page[MAX_ARG_PAGES]; + void **page; + int n_arg_pages; abi_ulong p; int fd; int e_uid, e_gid; diff --git a/linux-user/syscall.c b/linux-user/syscall.c index f62c698..6fa5fb4 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -5799,12 +5799,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, } *q = NULL; +/* This check is bogus. MAX_ARG_PAGES is a thing of the past. Trust the host +execve to set errno appropriately */ +#if 0 /* This case will not be caught by the host's execve() if its page size is bigger than the target's. */ if (total_size > MAX_ARG_PAGES * TARGET_PAGE_SIZE) { ret = -TARGET_E2BIG; goto execve_end; } +#endif if (!(p = lock_user_string(arg1))) goto execve_efault; ret = get_errno(execve(p, argp, envp));
qemu currently limits the space for the evironment and arguments to 32 * PAGE_SIZE. Linux limits the argument space to 1/4 of the stack size. A program trying to detect this with a getrlimit(RLIMIT_STACK) syscall will typically get a much larger limit than qemus current 128kB. The current limit causes "Argument list too long" errors. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- linux-user/elfload.c | 29 ++++++++++++++++++----------- linux-user/linuxload.c | 7 ++++--- linux-user/qemu.h | 11 ++--------- linux-user/syscall.c | 4 ++++ 4 files changed, 28 insertions(+), 23 deletions(-)