Re: [PATCH v2] linux-user: fix getgroups/setgroups allocations

[Laurent Vivier <laurent@vivier.eu>]

Le 17/12/2022 à 10:31, 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>
> ---
> 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..8a2f49d18f 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) && ret > 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) && ret > 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.0 branch.

Thanks,
Laurent