| Message ID | 1359846288-10053-1-git-send-email-dillona@dillona.com |
|---|---|
| State | New |
| Headers | show |
On 2 February 2013 23:04, <dillona@dillona.com> wrote: > From: Dillon Amburgey <dillona@dillona.com> > > Signed-off-by: Dillon Amburgey <dillona@dillona.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> -- PMM
On 02/02/2013 04:04 PM, dillona@dillona.com wrote: > From: Dillon Amburgey <dillona@dillona.com> > > Signed-off-by: Dillon Amburgey <dillona@dillona.com> > --- > linux-user/syscall.c | 22 ++++++++++++---------- > 1 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index a148d9f..7344052 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -7653,18 +7653,20 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > { > int gidsetsize = arg1; > target_id *target_grouplist; > - gid_t *grouplist; > + gid_t *grouplist = NULL; > int i; > - > - grouplist = alloca(gidsetsize * sizeof(gid_t)); > - target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * 2, 1); > - if (!target_grouplist) { > - ret = -TARGET_EFAULT; > - goto fail; > + if (gidsetsize) { > + grouplist = alloca(gidsetsize * sizeof(gid_t)); Is this alloca() safe, or are you risking stack overflow if the user passes an extremely large arg1?
On 4 February 2013 18:38, Eric Blake <eblake@redhat.com> wrote: > On 02/02/2013 04:04 PM, dillona@dillona.com wrote: >> - >> - grouplist = alloca(gidsetsize * sizeof(gid_t)); >> - target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * 2, 1); >> - if (!target_grouplist) { >> - ret = -TARGET_EFAULT; >> - goto fail; >> + if (gidsetsize) { >> + grouplist = alloca(gidsetsize * sizeof(gid_t)); > > Is this alloca() safe, or are you risking stack overflow if the user > passes an extremely large arg1? No, the linux-user has a number of long-standing not-terribly-safe alloca calls like this. If anybody wants to go through and fix them patches are welcome, but I don't think it's fair to require them to be fixed in order to get fairly simple patches like this in, where the patch is merely reindenting existing dubious code, not adding to the problem. -- PMM
On 02/04/2013 02:07 PM, Peter Maydell wrote: > On 4 February 2013 18:38, Eric Blake <eblake@redhat.com> wrote: >> On 02/02/2013 04:04 PM, dillona@dillona.com wrote: >>> - >>> - grouplist = alloca(gidsetsize * sizeof(gid_t)); >>> - target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * 2, 1); >>> - if (!target_grouplist) { >>> - ret = -TARGET_EFAULT; >>> - goto fail; >>> + if (gidsetsize) { >>> + grouplist = alloca(gidsetsize * sizeof(gid_t)); >> >> Is this alloca() safe, or are you risking stack overflow if the user >> passes an extremely large arg1? > > No, the linux-user has a number of long-standing not-terribly-safe > alloca calls like this. If anybody wants to go through and fix them > patches are welcome, but I don't think it's fair to require them > to be fixed in order to get fairly simple patches like this in, > where the patch is merely reindenting existing dubious code, not > adding to the problem. Point taken - the abuse of alloca() is pre-existing, so it shouldn't block this particular patch.
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index a148d9f..7344052 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7653,18 +7653,20 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, { int gidsetsize = arg1; target_id *target_grouplist; - gid_t *grouplist; + gid_t *grouplist = NULL; int i; - - grouplist = alloca(gidsetsize * sizeof(gid_t)); - target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * 2, 1); - if (!target_grouplist) { - ret = -TARGET_EFAULT; - goto fail; + if (gidsetsize) { + grouplist = alloca(gidsetsize * sizeof(gid_t)); + target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * 2, 1); + if (!target_grouplist) { + ret = -TARGET_EFAULT; + goto fail; + } + for (i = 0; i < gidsetsize; i++) { + grouplist[i] = low2highgid(tswapid(target_grouplist[i])); + } + unlock_user(target_grouplist, arg2, 0); } - for(i = 0;i < gidsetsize; i++) - grouplist[i] = low2highgid(tswapid(target_grouplist[i])); - unlock_user(target_grouplist, arg2, 0); ret = get_errno(setgroups(gidsetsize, grouplist)); } break;