* [PATCH v2] linux-user: fix getgroups/setgroups allocations
@ 2022-12-17 9:31 Michael Tokarev
2022-12-17 18:55 ` Richard Henderson
2023-01-25 13:18 ` Laurent Vivier
0 siblings, 2 replies; 8+ messages in thread
From: Michael Tokarev @ 2022-12-17 9:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Michael Tokarev
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
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] linux-user: fix getgroups/setgroups allocations
2022-12-17 9:31 [PATCH v2] linux-user: fix getgroups/setgroups allocations Michael Tokarev
@ 2022-12-17 18:55 ` Richard Henderson
2023-01-25 13:18 ` Laurent Vivier
1 sibling, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2022-12-17 18:55 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel; +Cc: Laurent Vivier
On 12/17/22 01:31, 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.
>
> 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(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] linux-user: fix getgroups/setgroups allocations
2022-12-17 9:31 [PATCH v2] linux-user: fix getgroups/setgroups allocations Michael Tokarev
2022-12-17 18:55 ` Richard Henderson
@ 2023-01-25 13:18 ` Laurent Vivier
2023-02-03 21:49 ` Laurent Vivier
1 sibling, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2023-01-25 13:18 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] linux-user: fix getgroups/setgroups allocations
2023-01-25 13:18 ` Laurent Vivier
@ 2023-02-03 21:49 ` Laurent Vivier
2023-02-03 21:57 ` Richard Henderson
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2023-02-03 21:49 UTC (permalink / raw)
To: Michael Tokarev; +Cc: qemu-devel, Richard Henderson
On 1/25/23 14:18, Laurent Vivier wrote:
> 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.
I'm going to remove this patch from my branch because it breaks something.
When I execute LTP test suite (20200930), I have:
getgroups01 1 TPASS : getgroups failed as expected with EINVAL
**
ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion failed:
(cpu == current_cpu)
Bail out! ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion
failed: (cpu == current_cpu)
**
ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion failed:
(cpu == current_cpu)
Bail out! ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion
failed: (cpu == current_cpu)
Thanks,
Laurent
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] linux-user: fix getgroups/setgroups allocations
2023-02-03 21:49 ` Laurent Vivier
@ 2023-02-03 21:57 ` Richard Henderson
2023-02-03 22:23 ` Laurent Vivier
0 siblings, 1 reply; 8+ messages in thread
From: Richard Henderson @ 2023-02-03 21:57 UTC (permalink / raw)
To: Laurent Vivier, Michael Tokarev; +Cc: qemu-devel
On 2/3/23 11:49, Laurent Vivier wrote:
> On 1/25/23 14:18, Laurent Vivier wrote:
>> 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.
>
> I'm going to remove this patch from my branch because it breaks something.
>
> When I execute LTP test suite (20200930), I have:
>
> getgroups01 1 TPASS : getgroups failed as expected with EINVAL
> **
> ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion failed:
> (cpu == current_cpu)
> Bail out! ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion
> failed: (cpu == current_cpu)
Which host+guest?
I've had several reports of this, but can't replicate it locally. The cpu_exec_setjmp
function is now quite small and easy to analyze -- the only way I can see that this could
fail is if there is some stack smashing issue...
r~
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] linux-user: fix getgroups/setgroups allocations
2023-02-03 21:57 ` Richard Henderson
@ 2023-02-03 22:23 ` Laurent Vivier
2023-04-09 9:06 ` Michael Tokarev
[not found] ` <7a3beebe-5593-8fda-f8ec-7e08789da12f@tls.msk.ru>
0 siblings, 2 replies; 8+ messages in thread
From: Laurent Vivier @ 2023-02-03 22:23 UTC (permalink / raw)
To: qemu-devel
Le 03/02/2023 à 22:57, Richard Henderson a écrit :
> On 2/3/23 11:49, Laurent Vivier wrote:
>> On 1/25/23 14:18, Laurent Vivier wrote:
>>> 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.
>>
>> I'm going to remove this patch from my branch because it breaks something.
>>
>> When I execute LTP test suite (20200930), I have:
>>
>> getgroups01 1 TPASS : getgroups failed as expected with EINVAL
>> **
>> ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion failed: (cpu ==
>> current_cpu)
>> Bail out! ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion failed:
>> (cpu == current_cpu)
>
> Which host+guest?
host is x86_64 + Fedora 37 (QEMU needs to be patched to detect missing libdw static library)
guest is all, replicated with m68k/sid
Note that the error is generated with the test case that expects EINVAL.
Thanks,
Laurent
>
> I've had several reports of this, but can't replicate it locally. The cpu_exec_setjmp function is
> now quite small and easy to analyze -- the only way I can see that this could fail is if there is
> some stack smashing issue...
>
>
> r~
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] linux-user: fix getgroups/setgroups allocations
2023-02-03 22:23 ` Laurent Vivier
@ 2023-04-09 9:06 ` Michael Tokarev
[not found] ` <7a3beebe-5593-8fda-f8ec-7e08789da12f@tls.msk.ru>
1 sibling, 0 replies; 8+ messages in thread
From: Michael Tokarev @ 2023-04-09 9:06 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
04.02.2023 01:23, Laurent Vivier wrote:
> Le 03/02/2023 à 22:57, Richard Henderson a écrit :
>> On 2/3/23 11:49, Laurent Vivier wrote:
..
>>> I'm going to remove this patch from my branch because it breaks something.
>>>
>>> When I execute LTP test suite (20200930), I have:
>>>
>>> getgroups01 1 TPASS : getgroups failed as expected with EINVAL
>>> **
>>> ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion failed: (cpu == current_cpu)
>>> Bail out! ERROR:../../../Projects/qemu/accel/tcg/cpu-exec.c:998:cpu_exec_setjmp: assertion failed: (cpu == current_cpu)
>>
>> Which host+guest?
>
> host is x86_64 + Fedora 37 (QEMU needs to be patched to detect missing libdw static library)
> guest is all, replicated with m68k/sid
>
> Note that the error is generated with the test case that expects EINVAL.
Hm. I missed this one at its time. The patch's in debian qemu for quite some time
without any issues so far.
Laurent, can you describe what you're doing there in a bit more details please?
I'd love to fix this one. Do you know which test is/was failing?
Thanks,
/mjt
^ permalink raw reply [flat|nested] 8+ messages in thread[parent not found: <7a3beebe-5593-8fda-f8ec-7e08789da12f@tls.msk.ru>]
end of thread, other threads:[~2023-04-09 9:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-17 9:31 [PATCH v2] linux-user: fix getgroups/setgroups allocations Michael Tokarev
2022-12-17 18:55 ` Richard Henderson
2023-01-25 13:18 ` Laurent Vivier
2023-02-03 21:49 ` Laurent Vivier
2023-02-03 21:57 ` Richard Henderson
2023-02-03 22:23 ` Laurent Vivier
2023-04-09 9:06 ` Michael Tokarev
[not found] ` <7a3beebe-5593-8fda-f8ec-7e08789da12f@tls.msk.ru>
2023-04-09 9:17 ` Michael Tokarev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).