From: Dr. David Alan Gilbert <dave@treblig.org>
Date: Tue, 5 Nov 2013 10:54:51 +0000 (+0100)
Subject: Fix select decoding with bogus (huge or negative) nfds.
X-Git-Tag: v4.9~179
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=025f1082b6c9573772472cc9039c2e10225c2c42;p=strace

Fix select decoding with bogus (huge or negative) nfds.

We used to allocate and fetch bit arrays using a sanitized
length, but then iterate over them with "j < arg[0]" condition,
where arg[0] is not sanitized. This segfaults if arg[0] is huge
or negative. This change fixes this.

Add test/select.c to capture the case.

Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---

diff --git a/desc.c b/desc.c
index 9bfe4d00..384b1472 100644
--- a/desc.c
+++ b/desc.c
@@ -492,14 +492,17 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
 		fdsize = 1024*1024;
 	if (args[0] < 0)
 		fdsize = 0;
+	nfds = fdsize;
 	fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long);
+	/* We had bugs a-la "while (j < args[0])" and "umoven(args[0])" below.
+	 * Instead of args[0], use nfds for fd count, fdsize for array lengths.
+	 */
 
 	if (entering(tcp)) {
 		fds = malloc(fdsize);
 		if (!fds)
 			die_out_of_memory();
-		nfds = args[0];
-		tprintf("%d", nfds);
+		tprintf("%ld", args[0]);
 		for (i = 0; i < 3; i++) {
 			arg = args[i+1];
 			if (arg == 0) {
@@ -533,12 +536,13 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
 		char *outptr;
 #define end_outstr (outstr + sizeof(outstr))
 		const char *sep;
+		long ready_fds;
 
 		if (syserror(tcp))
 			return 0;
 
-		nfds = tcp->u_rval;
-		if (nfds == 0) {
+		ready_fds = tcp->u_rval;
+		if (ready_fds == 0) {
 			tcp->auxstr = "Timeout";
 			return RVAL_STR;
 		}
@@ -555,7 +559,7 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
 			arg = args[i+1];
 			if (!arg || umoven(tcp, arg, fdsize, (char *) fds) < 0)
 				continue;
-			for (j = 0; j < args[0]; j++) {
+			for (j = 0; j < nfds; j++) {
 				if (FD_ISSET(j, fds)) {
 					/* +2 chars needed at the end: ']',NUL */
 					if (outptr < end_outstr - (sizeof(", except [") + sizeof(int)*3 + 2)) {
@@ -572,12 +576,12 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
 							outptr += sprintf(outptr, " %u", j);
 						}
 					}
-					nfds--;
+					ready_fds--;
 				}
 			}
 			if (outptr != outstr)
 				*outptr++ = ']';
-			if (nfds == 0)
+			if (ready_fds == 0)
 				break;
 		}
 		free(fds);
diff --git a/test/.gitignore b/test/.gitignore
index 7eb39cf4..c73b64a7 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -10,4 +10,5 @@ wait_must_be_interruptible
 threaded_execve
 mtd
 ubi
+select
 sigreturn
diff --git a/test/Makefile b/test/Makefile
index 92142b18..cc7d47a8 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,7 +3,7 @@ CFLAGS += -Wall
 PROGS = \
     vfork fork sig skodic clone leaderkill childthread \
     sigkill_rain wait_must_be_interruptible threaded_execve \
-    mtd ubi sigreturn
+    mtd ubi select sigreturn
 
 all: $(PROGS)
 
diff --git a/test/select.c b/test/select.c
new file mode 100644
index 00000000..523d75cb
--- /dev/null
+++ b/test/select.c
@@ -0,0 +1,23 @@
+/* dave@treblig.org */
+#include <sys/select.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+char buffer[1024*1024*2];
+
+int main()
+{
+	fd_set rds;
+	FD_ZERO(&rds);
+	FD_SET(2, &rds);
+	/* Start with a nice simple select */
+	select(3, &rds, &rds, &rds, NULL);
+	/* Now the crash case that trinity found, -ve nfds
+	 * but with a pointer to a large chunk of valid memory
+	 */
+	select(-1, (fd_set *)buffer, NULL, NULL, NULL);
+	return 0;
+}