linux-user/syscall.c | 99 ++++++++++++++++++++++++++++++-------------- 1 file changed, 68 insertions(+), 31 deletions(-)
linux-user getgroups(), setgroups(), getgroups32() and setgroups32()
used alloca() to allocate grouplist arrays, with unchecked gidsetsize
coming from the "guest". With NGROUPS_MAX being 65536 (linux, and it
is common for an application to allocate NGROUPS_MAX for getgroups()),
this means a typical allocation is half the megabyte on the stack.
Which just overflows stack, which leads to immediate SIGSEGV in actual
system getgroups() implementation.
An example of such issue is aptitude, eg
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=811087#72
Cap gidsetsize to NGROUPS_MAX (return EINVAL if it is larger than that),
and use heap allocation for grouplist instead of alloca(). While at it,
fix coding style and make all 4 implementations identical.
Try to not impose random limits - for example, allow gidsetsize to be
negative for getgroups() - just do not allocate negative-sized grouplist
in this case but still do actual getgroups() call. But do not allow
negative gidsetsize for setgroups() since its argument is unsigned.
Capping by NGROUPS_MAX seems a bit arbitrary, - we can do more, it is
not an error if set size will be NGROUPS_MAX+1. But we should not allow
integer overflow for the array being allocated. Maybe it is enough to
just call g_try_new() and return ENOMEM if it fails.
Maybe there's also no need to convert setgroups() since this one is
usually smaller and known beforehand (KERN_NGROUPS_MAX is actually 63, -
this is apparently a kernel-imposed limit for runtime group set).
The patch fixes aptitude segfault mentioned above.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
v4:
- the same ret-vs-gidsetsize fix in getgroups32.
v3:
- fix a bug in getgroups(). In initial implementation I checked
for ret>0 in order to convert returned list of groups to target
byte order. But this clashes with unusual corner case for this
syscall: getgroups(0,NULL) return current number of groups in
the set, so this resulted in writing to *NULL. The right condition
here is gidsetsize>0:
- if (!is_error(ret) && ret > 0) {
+ if (!is_error(ret) && gidsetsize > 0) {
v2:
- remove g_free, use g_autofree annotations instead,
- a bit more coding style changes, makes checkpatch.pl happy
linux-user/syscall.c | 99 ++++++++++++++++++++++++++++++--------------
1 file changed, 68 insertions(+), 31 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 24b25759be..c532ee92c1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11433,39 +11433,58 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
{
int gidsetsize = arg1;
target_id *target_grouplist;
- gid_t *grouplist;
+ g_autofree gid_t *grouplist = NULL;
int i;
- grouplist = alloca(gidsetsize * sizeof(gid_t));
+ if (gidsetsize > NGROUPS_MAX) {
+ return -TARGET_EINVAL;
+ }
+ if (gidsetsize > 0) {
+ grouplist = g_try_new(gid_t, gidsetsize);
+ if (!grouplist) {
+ return -TARGET_ENOMEM;
+ }
+ }
ret = get_errno(getgroups(gidsetsize, grouplist));
- if (gidsetsize == 0)
- return ret;
- if (!is_error(ret)) {
- target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * sizeof(target_id), 0);
- if (!target_grouplist)
+ if (!is_error(ret) && gidsetsize > 0) {
+ target_grouplist = lock_user(VERIFY_WRITE, arg2,
+ gidsetsize * sizeof(target_id), 0);
+ if (!target_grouplist) {
return -TARGET_EFAULT;
- for(i = 0;i < ret; i++)
+ }
+ for (i = 0; i < ret; i++) {
target_grouplist[i] = tswapid(high2lowgid(grouplist[i]));
- unlock_user(target_grouplist, arg2, gidsetsize * sizeof(target_id));
+ }
+ unlock_user(target_grouplist, arg2,
+ gidsetsize * sizeof(target_id));
}
+ return ret;
}
- return ret;
case TARGET_NR_setgroups:
{
int gidsetsize = arg1;
target_id *target_grouplist;
- gid_t *grouplist = NULL;
+ g_autofree gid_t *grouplist = NULL;
int i;
- if (gidsetsize) {
- grouplist = alloca(gidsetsize * sizeof(gid_t));
- target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * sizeof(target_id), 1);
+
+ if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) {
+ return -TARGET_EINVAL;
+ }
+ if (gidsetsize > 0) {
+ grouplist = g_try_new(gid_t, gidsetsize);
+ if (!grouplist) {
+ return -TARGET_ENOMEM;
+ }
+ target_grouplist = lock_user(VERIFY_READ, arg2,
+ gidsetsize * sizeof(target_id), 1);
if (!target_grouplist) {
return -TARGET_EFAULT;
}
for (i = 0; i < gidsetsize; i++) {
grouplist[i] = low2highgid(tswapid(target_grouplist[i]));
}
- unlock_user(target_grouplist, arg2, 0);
+ unlock_user(target_grouplist, arg2,
+ gidsetsize * sizeof(target_id));
}
return get_errno(setgroups(gidsetsize, grouplist));
}
@@ -11750,41 +11769,59 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
{
int gidsetsize = arg1;
uint32_t *target_grouplist;
- gid_t *grouplist;
+ g_autofree gid_t *grouplist = NULL;
int i;
- grouplist = alloca(gidsetsize * sizeof(gid_t));
+ if (gidsetsize > NGROUPS_MAX) {
+ return -TARGET_EINVAL;
+ }
+ if (gidsetsize > 0) {
+ grouplist = g_try_new(gid_t, gidsetsize);
+ if (!grouplist) {
+ return -TARGET_ENOMEM;
+ }
+ }
ret = get_errno(getgroups(gidsetsize, grouplist));
- if (gidsetsize == 0)
- return ret;
- if (!is_error(ret)) {
- target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 4, 0);
+ if (!is_error(ret) && gidsetsize > 0) {
+ target_grouplist = lock_user(VERIFY_WRITE, arg2,
+ gidsetsize * 4, 0);
if (!target_grouplist) {
return -TARGET_EFAULT;
}
- for(i = 0;i < ret; i++)
+ for (i = 0; i < ret; i++) {
target_grouplist[i] = tswap32(grouplist[i]);
+ }
unlock_user(target_grouplist, arg2, gidsetsize * 4);
}
+ return ret;
}
- return ret;
#endif
#ifdef TARGET_NR_setgroups32
case TARGET_NR_setgroups32:
{
int gidsetsize = arg1;
uint32_t *target_grouplist;
- gid_t *grouplist;
+ g_autofree gid_t *grouplist = NULL;
int i;
- grouplist = alloca(gidsetsize * sizeof(gid_t));
- target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * 4, 1);
- if (!target_grouplist) {
- return -TARGET_EFAULT;
+ if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) {
+ return -TARGET_EINVAL;
+ }
+ if (gidsetsize > 0) {
+ grouplist = g_try_new(gid_t, gidsetsize);
+ if (!grouplist) {
+ return -TARGET_ENOMEM;
+ }
+ target_grouplist = lock_user(VERIFY_READ, arg2,
+ gidsetsize * 4, 1);
+ if (!target_grouplist) {
+ return -TARGET_EFAULT;
+ }
+ for (i = 0; i < gidsetsize; i++) {
+ grouplist[i] = tswap32(target_grouplist[i]);
+ }
+ unlock_user(target_grouplist, arg2, 0);
}
- for(i = 0;i < gidsetsize; i++)
- grouplist[i] = tswap32(target_grouplist[i]);
- unlock_user(target_grouplist, arg2, 0);
return get_errno(setgroups(gidsetsize, grouplist));
}
#endif
--
2.30.2Le 09/04/2023 à 12:53, Michael Tokarev a écrit :
> linux-user getgroups(), setgroups(), getgroups32() and setgroups32()
> used alloca() to allocate grouplist arrays, with unchecked gidsetsize
> coming from the "guest". With NGROUPS_MAX being 65536 (linux, and it
> is common for an application to allocate NGROUPS_MAX for getgroups()),
> this means a typical allocation is half the megabyte on the stack.
> Which just overflows stack, which leads to immediate SIGSEGV in actual
> system getgroups() implementation.
>
> An example of such issue is aptitude, eg
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=811087#72
>
> Cap gidsetsize to NGROUPS_MAX (return EINVAL if it is larger than that),
> and use heap allocation for grouplist instead of alloca(). While at it,
> fix coding style and make all 4 implementations identical.
>
> Try to not impose random limits - for example, allow gidsetsize to be
> negative for getgroups() - just do not allocate negative-sized grouplist
> in this case but still do actual getgroups() call. But do not allow
> negative gidsetsize for setgroups() since its argument is unsigned.
>
> Capping by NGROUPS_MAX seems a bit arbitrary, - we can do more, it is
> not an error if set size will be NGROUPS_MAX+1. But we should not allow
> integer overflow for the array being allocated. Maybe it is enough to
> just call g_try_new() and return ENOMEM if it fails.
>
> Maybe there's also no need to convert setgroups() since this one is
> usually smaller and known beforehand (KERN_NGROUPS_MAX is actually 63, -
> this is apparently a kernel-imposed limit for runtime group set).
>
> The patch fixes aptitude segfault mentioned above.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v4:
> - the same ret-vs-gidsetsize fix in getgroups32.
> v3:
> - fix a bug in getgroups(). In initial implementation I checked
> for ret>0 in order to convert returned list of groups to target
> byte order. But this clashes with unusual corner case for this
> syscall: getgroups(0,NULL) return current number of groups in
> the set, so this resulted in writing to *NULL. The right condition
> here is gidsetsize>0:
> - if (!is_error(ret) && ret > 0) {
> + if (!is_error(ret) && gidsetsize > 0) {
> v2:
> - remove g_free, use g_autofree annotations instead,
> - a bit more coding style changes, makes checkpatch.pl happy
>
> linux-user/syscall.c | 99 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 68 insertions(+), 31 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 24b25759be..c532ee92c1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11433,39 +11433,58 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> {
> int gidsetsize = arg1;
> target_id *target_grouplist;
> - gid_t *grouplist;
> + g_autofree gid_t *grouplist = NULL;
> int i;
>
> - grouplist = alloca(gidsetsize * sizeof(gid_t));
> + if (gidsetsize > NGROUPS_MAX) {
> + return -TARGET_EINVAL;
> + }
> + if (gidsetsize > 0) {
> + grouplist = g_try_new(gid_t, gidsetsize);
> + if (!grouplist) {
> + return -TARGET_ENOMEM;
> + }
> + }
> ret = get_errno(getgroups(gidsetsize, grouplist));
> - if (gidsetsize == 0)
> - return ret;
> - if (!is_error(ret)) {
> - target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * sizeof(target_id), 0);
> - if (!target_grouplist)
> + if (!is_error(ret) && gidsetsize > 0) {
> + target_grouplist = lock_user(VERIFY_WRITE, arg2,
> + gidsetsize * sizeof(target_id), 0);
> + if (!target_grouplist) {
> return -TARGET_EFAULT;
> - for(i = 0;i < ret; i++)
> + }
> + for (i = 0; i < ret; i++) {
> target_grouplist[i] = tswapid(high2lowgid(grouplist[i]));
> - unlock_user(target_grouplist, arg2, gidsetsize * sizeof(target_id));
> + }
> + unlock_user(target_grouplist, arg2,
> + gidsetsize * sizeof(target_id));
> }
> + return ret;
> }
> - return ret;
> case TARGET_NR_setgroups:
> {
> int gidsetsize = arg1;
> target_id *target_grouplist;
> - gid_t *grouplist = NULL;
> + g_autofree gid_t *grouplist = NULL;
> int i;
> - if (gidsetsize) {
> - grouplist = alloca(gidsetsize * sizeof(gid_t));
> - target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * sizeof(target_id), 1);
> +
> + if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) {
> + return -TARGET_EINVAL;
> + }
> + if (gidsetsize > 0) {
> + grouplist = g_try_new(gid_t, gidsetsize);
> + if (!grouplist) {
> + return -TARGET_ENOMEM;
> + }
> + target_grouplist = lock_user(VERIFY_READ, arg2,
> + gidsetsize * sizeof(target_id), 1);
> if (!target_grouplist) {
> return -TARGET_EFAULT;
> }
> for (i = 0; i < gidsetsize; i++) {
> grouplist[i] = low2highgid(tswapid(target_grouplist[i]));
> }
> - unlock_user(target_grouplist, arg2, 0);
> + unlock_user(target_grouplist, arg2,
> + gidsetsize * sizeof(target_id));
> }
> return get_errno(setgroups(gidsetsize, grouplist));
> }
> @@ -11750,41 +11769,59 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> {
> int gidsetsize = arg1;
> uint32_t *target_grouplist;
> - gid_t *grouplist;
> + g_autofree gid_t *grouplist = NULL;
> int i;
>
> - grouplist = alloca(gidsetsize * sizeof(gid_t));
> + if (gidsetsize > NGROUPS_MAX) {
> + return -TARGET_EINVAL;
> + }
> + if (gidsetsize > 0) {
> + grouplist = g_try_new(gid_t, gidsetsize);
> + if (!grouplist) {
> + return -TARGET_ENOMEM;
> + }
> + }
> ret = get_errno(getgroups(gidsetsize, grouplist));
> - if (gidsetsize == 0)
> - return ret;
> - if (!is_error(ret)) {
> - target_grouplist = lock_user(VERIFY_WRITE, arg2, gidsetsize * 4, 0);
> + if (!is_error(ret) && gidsetsize > 0) {
> + target_grouplist = lock_user(VERIFY_WRITE, arg2,
> + gidsetsize * 4, 0);
> if (!target_grouplist) {
> return -TARGET_EFAULT;
> }
> - for(i = 0;i < ret; i++)
> + for (i = 0; i < ret; i++) {
> target_grouplist[i] = tswap32(grouplist[i]);
> + }
> unlock_user(target_grouplist, arg2, gidsetsize * 4);
> }
> + return ret;
> }
> - return ret;
> #endif
> #ifdef TARGET_NR_setgroups32
> case TARGET_NR_setgroups32:
> {
> int gidsetsize = arg1;
> uint32_t *target_grouplist;
> - gid_t *grouplist;
> + g_autofree gid_t *grouplist = NULL;
> int i;
>
> - grouplist = alloca(gidsetsize * sizeof(gid_t));
> - target_grouplist = lock_user(VERIFY_READ, arg2, gidsetsize * 4, 1);
> - if (!target_grouplist) {
> - return -TARGET_EFAULT;
> + if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) {
> + return -TARGET_EINVAL;
> + }
> + if (gidsetsize > 0) {
> + grouplist = g_try_new(gid_t, gidsetsize);
> + if (!grouplist) {
> + return -TARGET_ENOMEM;
> + }
> + target_grouplist = lock_user(VERIFY_READ, arg2,
> + gidsetsize * 4, 1);
> + if (!target_grouplist) {
> + return -TARGET_EFAULT;
> + }
> + for (i = 0; i < gidsetsize; i++) {
> + grouplist[i] = tswap32(target_grouplist[i]);
> + }
> + unlock_user(target_grouplist, arg2, 0);
> }
> - for(i = 0;i < gidsetsize; i++)
> - grouplist[i] = tswap32(target_grouplist[i]);
> - unlock_user(target_grouplist, arg2, 0);
> return get_errno(setgroups(gidsetsize, grouplist));
> }
> #endif
Applied to my linux-user-for-8.1 branch.
Thanks,
Laurent
09.04.2023 13:53, Michael Tokarev wrote: > linux-user getgroups(), setgroups(), getgroups32() and setgroups32() > used alloca() to allocate grouplist arrays, with unchecked gidsetsize > coming from the "guest". With NGROUPS_MAX being 65536 (linux, and it > is common for an application to allocate NGROUPS_MAX for getgroups()), > this means a typical allocation is half the megabyte on the stack. > Which just overflows stack, which leads to immediate SIGSEGV in actual > system getgroups() implementation. Ping? Thanks, /mjt
© 2016 - 2024 Red Hat, Inc.