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 9bfe4d00b918570c110f55603e75814f079697b8..384b1472031112dbeb2b37dd0b510922d12244c7 100644 (file)
--- 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 7eb39cf492d4327bf43e310a1183c1423d0053dd..c73b64a761434f4b4d4186016041fa65d48e9090 100644 (file)
--- 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 92142b180ee3762b74f1a70ea44ccd9dcc339f4c..cc7d47a8e8430e0f28634bca42c3e7e12892ecd2 100644 (file)
--- 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 (file)
index 0000000..523d75c
--- /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;
+}