Patchwork linux-user: Support setgroups syscall with no groups

login
register
mail settings
Submitter dillona@dillona.com
Date Feb. 2, 2013, 11:04 p.m.
Message ID <1359846288-10053-1-git-send-email-dillona@dillona.com>
Download mbox | patch
Permalink /patch/217708/
State New
Headers show

Comments

dillona@dillona.com - Feb. 2, 2013, 11:04 p.m.
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(-)
Peter Maydell - Feb. 2, 2013, 11:23 p.m.
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
Eric Blake - Feb. 4, 2013, 6:38 p.m.
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?
Peter Maydell - Feb. 4, 2013, 9:07 p.m.
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
Eric Blake - Feb. 4, 2013, 9:25 p.m.
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.

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;