strace seg with select and -ve nfds

[Dr. David Alan Gilbert]

The 'trinity' fuzz tester managed to trigger a seg of strace
when doing a select() with a -ve nfds value but pointing to a valid large
buffer (on x86 Linux).

My patch below fixes this; I'm not 100% happy because:
  1) It seems way too complicated - can't we quit earlier when we find
     the length is weird?
  2) It's odd the way the code reads the arg for fdsize and then later
     reads it again for nfds, I think that's really the underlying
     reason this tripped.
  3) I'd like some reassurance that my understanding of the way
     strace's arg types work is right.

(WTH does strace use int for nfds?)

Thoughts?

Dave

commit 9354b400e243cc87f8b2964c4220afcbd077fb30
Author: Dr. David Alan Gilbert <dave at treblig.org>
Date:   Mon Nov 4 21:45:24 2013 +0000

    Fix select() with -ve nfds
    
    select() with a -ve nfds value but a valid buffer seg'd.
    Work with the nfds parameters as an int everywhere (which
    it is) and flag when we fixup the value so that we don't then
    try and walk the buffer.
    
    Also adds test/select.c to capture the case.

diff --git a/desc.c b/desc.c
index 9bfe4d0..30ff7a7 100644
--- a/desc.c
+++ b/desc.c
@@ -481,24 +481,29 @@ static int
 decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
 {
 	int i, j;
-	unsigned nfds, fdsize;
+	int nfds, fdsize;
 	fd_set *fds;
 	const char *sep;
 	long arg;
+	int unusual_nfds = 0;
 
-	fdsize = args[0];
+	fdsize = (int)args[0];
 	/* Beware of select(2^31-1, NULL, NULL, NULL) and similar... */
-	if (args[0] > 1024*1024)
+	if (fdsize > 1024*1024) {
 		fdsize = 1024*1024;
-	if (args[0] < 0)
+		unusual_nfds = 1;
+	}
+	if (fdsize < 0) {
 		fdsize = 0;
+		unusual_nfds = 1;
+	}
 	fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long);
 
 	if (entering(tcp)) {
 		fds = malloc(fdsize);
 		if (!fds)
 			die_out_of_memory();
-		nfds = args[0];
+		nfds = (int)args[0];
 		tprintf("%d", nfds);
 		for (i = 0; i < 3; i++) {
 			arg = args[i+1];
@@ -510,7 +515,8 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
 				tprintf(", %#lx", arg);
 				continue;
 			}
-			if (umoven(tcp, arg, fdsize, (char *) fds) < 0) {
+			if ((umoven(tcp, arg, fdsize, (char *) fds) < 0) ||
+			    unusual_nfds) {
 				tprints(", [?]");
 				continue;
 			}
diff --git a/test/.gitignore b/test/.gitignore
index 7eb39cf..c73b64a 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 92142b1..cc7d47a 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 0000000..fba54b3
--- /dev/null
+++ b/test/select.c
@@ -0,0 +1,31 @@
+/* dave at treblig.org */
+
+#include <sys/select.h>
+
+#include <sys/time.h>
+#include <sys/types.h>
+
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#define BUFSIZE 1024*1024*2
+char buffer[BUFSIZE];
+
+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);
+
+  exit(0);
+}
+
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\ gro.gilbert @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/