[PATCH] NGROUPS_MAX will cause stack overflow

[Peter S. Mazinger]

On Fri, 16 Dec 2005, Aubrey wrote:

> On 12/15/05, Robin Getz <rgetz at blackfin.uclinux.org> wrote:
> > Mike Frysinger wrote:
> > >Robin Getz forwarded the report to us earlier
> >
> > I asked Aubrey to make the patch - as he was the one who found the the
> > issue - he should be the person who makes the patch, & gets his name in the
> > contributors file.
> >
> 
> Oh, it doesn't matter to me, thanks for Robin. I just want to make
> things better as others.
> 
> > Aubrey/Mike:
> >
> > I think the same issue exists in
> > ./uClibc/libc/sysdeps/linux/common/setgroups.c:setgroups
> >
> > which is used in many uClinux user space applications. Can you confirm you
> > see the same issue? If so, Aubrey, can you make/test/send a patch?
> >
> > Thanks
> > -Robin
> >
> Yes, I checked it. And I found not only "setgroups.c" but
> "getgroups.c" have the matrix (__kernel_gid_t kernel_groups[n]) on the
> stack which can be very large because "n" can be assigned to
> NGROUPS_MAX. I also changed it to do malloc. The following is the
> patch:

Which malloc implementation was tested, uClibc has 3 of them?

Peter

> =========================================================
> 2005-12-16 Aubrey.li <aubreylee at gmail.com>
>                 * Using malloc to alloc memory instead of a possible
> big matrix on the stack in case of stack overflow
> 
> Index: libc/sysdeps/linux/common/setgroups.c
>           libc/sysdeps/linux/common/getgroups.c
> 
> --- setgroups.c	2005-12-16 16:28:28.000000000 +0800
> +++ setgroups.c	2005-12-16 16:26:00.000000000 +0800
> @@ -22,7 +22,13 @@
>  		return -1;
>  	} else {
>  		size_t i;
> -		__kernel_gid_t kernel_groups[n];
> +		int ngids;
> +		__kernel_gid_t *kernel_groups;
> +
> +		if(kernel_groups=(__kernel_gid_t *)malloc(sizeof(__kernel_gid_t)*n) == NULL){
> +			__set_errno(EINVAL);
> +			return -1;
> +		}
> 
>  		for (i = 0; i < n; i++) {
>  			kernel_groups[i] = (groups)[i];
> @@ -31,6 +37,8 @@
>  				return -1;
>  			}
>  		}
> -		return (__syscall_setgroups(n, kernel_groups));
> +		ngids = __syscall_setgroups(n, kernel_groups);
> +		free(kernel_groups);
> +		return ngids;
>  	}
>  }
> --- getgroups.c	2005-12-16 16:28:43.000000000 +0800
> +++ getgroups.c	2005-12-16 16:24:21.000000000 +0800
> @@ -23,14 +23,21 @@
>  		return -1;
>  	} else {
>  		int i, ngids;
> -		__kernel_gid_t kernel_groups[n = MIN(n, sysconf(_SC_NGROUPS_MAX))];
> +		__kernel_gid_t *kernel_groups;
> 
> +		n = MIN(n, sysconf(_SC_NGROUPS_MAX));
> +		if(kernel_groups=(__kernel_gid_t *)malloc(sizeof(__kernel_gid_t)*n) == NULL){
> +			__set_errno(EINVAL);
> +			return -1;
> +		}
> +			
>  		ngids = __syscall_getgroups(n, kernel_groups);
>  		if (n != 0 && ngids > 0) {
>  			for (i = 0; i < ngids; i++) {
>  				groups[i] = kernel_groups[i];
>  			}
>  		}
> +		free(kernel_groups);
>  		return ngids;
>  	}
>  }
> 
> Thanks,
> -Aubrey
> 
> 

-- 
Peter S. Mazinger <ps dot m at gmx dot net>           ID: 0xA5F059F2
Key fingerprint = 92A4 31E1 56BC 3D5A 2D08  BB6E C389 975E A5F0 59F2