[PATCH] NGROUPS_MAX will cause stack overflow

[Aubrey]

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:
=========================================================
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