strace seg with select and -ve nfds

[Dr. David Alan Gilbert]

* Denys Vlasenko (dvlasenk at redhat.com) wrote:
> On 11/04/2013 10:39 PM, Dr. David Alan Gilbert wrote:

Hi Denys,
  Thanks for the reply,

> > 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?
> 
> Yes, it can be simpler.
> 
> >   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.
> 
> I propose to do simply this:
> 
> +       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.
> +        */
> 
> and use nfds in those two places where we incorrectly use arg[0] now.

> >   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?)
> 
> Because having more than 2^31-1 file descriptors in one process is insanity.

Only twice as insane as having 2^30-1 file descriptors!

> > Thoughts?
> 
> I applied a slightly simplified version of your fix to strace git, please try it.

That still fails (this is FORTIFY detecting the fail).

I can see two things potentially; the simple one is that the types are still
wrong so that the -1 starts out as 2^32-1 and goes through the route of being
corrected to be 1024*1024 rather than 0.

However, the other thing I think is being caught here is that fortify is 
catching FD_ISSET on a value greater than the size of the fd_set,
so the added test below causes it to hit that failure - I guess the
only solution there is not to use FD_ISSET or to cap at max-fds rather than
the arbitrary 1024*1024 ?
(I guess you could argue that's a false positive from fortify, but there
again I think it is an illegal use of FD_ISSET).

Anyway, here is the signedness fix:

diff --git a/desc.c b/desc.c
index 384b147..92cdf48 100644
--- a/desc.c
+++ b/desc.c
@@ -481,16 +481,16 @@ 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;
 
-       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)
+       if (fdsize < 0)
                fdsize = 0;
        nfds = fdsize;
        fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long);
@@ -502,7 +502,7 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
                fds = malloc(fdsize);
                if (!fds)
                        die_out_of_memory();
-               tprintf("%ld", args[0]);
+               tprintf("%d", (int)args[0]);
                for (i = 0; i < 3; i++) {
                        arg = args[i+1];
                        if (arg == 0) {

diff --git a/test/select.c b/test/select.c
index aee9f43..09f29c9 100644
--- a/test/select.c
+++ b/test/select.c
@@ -11,6 +11,8 @@ char buffer[1024*1024*2];
 int main()
 {
        fd_set rds;
+       struct timeval timeout;
+
        FD_ZERO(&rds);
        FD_SET(2, &rds);
        /* Start with a nice simple select */
@@ -18,6 +20,15 @@ int main()
        /* Now the crash case that trinity found, negative nfds
         * but with a pointer to a large chunk of valid memory.
         */
+       FD_ZERO((fd_set*)buffer);
+       FD_SET(2,(fd_set*)buffer);
        select(-1, (fd_set *)buffer, NULL, NULL, NULL);
+
+       /* Another variant, a large +ve value larger than
+        * the allowed number of fds.
+        */
+       timeout.tv_sec = 0;
+       timeout.tv_usec = 100;
+       select(10000, (fd_set *)buffer, NULL, NULL, &timeout);
        return 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   |_______/