Bug (+fix) in getdents() [glibc-2.3.2/linux-2.4.22/i686]

[Tsafrir Dan]

From: dants
To: address@hidden
Subject: Bug (+fix) in getdents() [glibc-2.3.2/linux-2.4.22/i686]

>Submitter-Id: Dan Tsafrir
>Confidential: no
>Synopsis:     The kernel's dirent's `d_off' field is unreliable. The effect
               of this on getdents(3) is:

               1- getdents falsely identifies "overflow" situation (where the
                  data of the kernel doesn't fit into the user's buffer).
               2- on "overflow" (true or false), getdents lseek()s to the
                  wrong offset.
               3- this leads the next getdents invocation to return erroneous
                  data to readdir(3) and possibly cause an endless loop in a
                  a program that expects readdir(3) to return NULL upon EOF.

               Another two minor bugs related to buffer allocation are also
               reported.

>Severity:     serious
>Priority:     high
>Category:     libc
>Class:        sw-bug
               + doc-bug
>Release:      libc-2.3.2
>Environment:  i686-pc-linux-gnu, linux-2.4.22, libranet-2.8, gcc-3.3.2

>Description:
#############

- The following relates to the file:
  glibc-2.3.2/sysdeps/unix/sysv/linux/getdents.c

- In this file, in the implementation of getdents(), the `d_off' field
  (belonging to the kernel's dirent structure) is falsely assumed to
  contain the offset to the next dirent.

- Note that the manual of libc's readdir(3) and the system call readdir(2)
  conflict regarding the content of `d_off':
  o readdir(3): "offset to the next dirent"
  o readdir(2): "offset to this dirent"

- In practice, both of the above are wrong. The `d_off' field may contain
  illegal negative values, 0 (should also never happen as the "next"
  dirent's offset must always be bigger then 0), or positive values that
  are bigger than anything a 32bit integer may hold (and are certainly
  bigger than the size of the directory file itself). Of course these values
  are anything else but the correct offsets. This is true both for local
  and remote (nfs/amd) file systems:

  o We're not sure what the Linux kernel intended to place in this field,
    but our experience shows that on "real" file systems (that actually
    reside on some disk) the offset seems to be a simple (not necessarily
    continuous) counter: e.g. first entry may have d_off=1, second: d_off=2,
    third: d_off=4096, fourth=d_off=4097 etc. We conjecture this is the
    serial of the dirent record within the directory.

  o For file systems that are maintained by the amd automounter (auto mount,
    directories) the d_off seems to be arbitrary (and may be negative, zero
    or beyond the scope of a 32bit integer). We conjecture the amd doesn't
    assign this field and the received values are simply garbage.

- The bug in libc only occurs when an "overflow" is detected (see
  lines: 176-183 in getdents.c).
  Overflow may happen when the kernel uses `kernel_dirent64' that
  contains 64bit wide fields (d_ino & d_off), while the user uses
  `struct dirent' (rather than `struct dirent64') through readdir(3),
  in which the corresponding fields are 32bit wide.

- In such a situation, getdents() tries to lseek (the directory-fd)
  to the end of the last-legal-dirent that getdents() has successfully
  read.
  This offset is supposedly held by the local variable `last_offset'.
  But, since `last_offset' is assigned with `d_off' on each iteration,
  and since as mentioned above `d_off' usually holds an incorrect value,
  the lseek is not performed to the correct place.
  getdents() then returns the number of bytes successfully read.

- This situation of course causes the next invocation of readdir() to
  return erroneous data due to the incorrect offset associated with
  the fd.
  FURTHERMORE, on our system, it often happens (for autofs directories
  that serve as auto mount points and are maintained by the amd) that the
  first dirent-entry has d_off=0 and the second dirent causes "overflow".
  In this case getdents() will return the number of bytes composing
  the first successfully read dirent, and will wrongly lseek to 0: the
  beginning of the file.
  This means that the next invocation of readdir(3) will return (again)
  the first dirent, and so on (never returning NULL), causing any
  program that uses readdir(3) to enter into an endless loop!

>How-To-Repeat:
###############

- The above bug will be reproduced whenever getdents() identifies a
  real overflow situation: this will happen if the user uses readdir(3)
  (rather than readdir64) and the the offsets in the directory are
  too big to be represented using `long'.

- However, as mentioned above, the program:

        DIR           *dir = opendir("some-autofs-directory");
        struct dirent *e;
        while( (e = readdir(dir)) != NULL )
                printf("ino=%d name=%s\n", e->d_ino, e->d_name)

  will enter an endless loop in a non-deterministic manner (depending
  on the garbage values returned by the amd automounter in the d_off
  field).

>Fix:
#####

1:---------------------------------------------------------------------
Instead of line 193:
        last_offset = d_off;
write:
        if( last_offset == -1 )
                last_offset = 0;
        last_offset += old_reclen;

    [  This makes `last_offset' hold the real last offset, and prevents
       the wrong lseek() when an "overflow" occurs. It works since the
       `d_reclen' is reliable and correct, as reflected by the manner
       getdents() uses `old_reclen'  ]


2:---------------------------------------------------------------------
The condition that identifies an "overflow" situation should also be
changed:
Instead of lines 176-179:
        if ((sizeof (outp->u.d_ino) != sizeof (inp->k.d_ino)
             && outp->u.d_ino != d_ino)
            || (sizeof (outp->u.d_off) != sizeof (inp->k.d_off)
                && outp->u.d_off != d_off))
write:
        if ( sizeof (outp->u.d_ino) != sizeof (inp->k.d_ino)
             && outp->u.d_ino != d_ino )

    [  We remove the right-side of the `or': since the `d_off' field is
       buggy and unusable, don't include it in the check for overflow.
       The alterative is that programs reading the content of a directory
       maintained by the amd using readdir(3), will usually fail due to
       overflow, even though the directory is prefectly fine.  ]


3:---------------------------------------------------------------------
The following is another bug we think you have (which is unrelated
to the above).
Instead of line 120:
        && nbytes <= sizeof (DIRENT_TYPE))
write:
        && nbytes <= sizeof (struct kernel_dirent64))

    [  The above condition decides whether the user gave a large enough
       buffer in which `struct kernel_dirent64' will fit (`nbytes' is the
       size of the user's `buf'). If `buf' is too small, a bigger buffer
       is allocated (lines 122-124) so as to be able to hold `struct
       kernel_dirent64'  ]


4:---------------------------------------------------------------------
We think the following is also an (unrelated) bug:
In lines 216-221:

        red_nbytes = MIN (nbytes
                      - ((nbytes / (offsetof (DIRENT_TYPE, d_name) + 14))
                         * size_diff),
                      nbytes - size_diff);

        skdp = kdp = __alloca (red_nbytes);

it is obvious that `red_nbytes' may be negative (or rather, wrapped-around),
since `nbytes' is a value given by the user (and may even be 0).
-----------------------------------------------------------------------


The above patch is given in the attached `getdents.diff-u'
Thanks,
    --Dan

--- getdents.c.orig     2003-12-04 19:11:38.000000000 +0200
+++ getdents.c  2003-12-04 19:38:06.000000000 +0200
@@ -117,7 +117,7 @@
       size_t kbytes = nbytes;
       if (offsetof (DIRENT_TYPE, d_name)
          < offsetof (struct kernel_dirent64, d_name)
-         && nbytes <= sizeof (DIRENT_TYPE))
+         && nbytes <= sizeof (kernel_dirent64))
        {
          kbytes = nbytes + offsetof (struct kernel_dirent64, d_name)
                   - offsetof (DIRENT_TYPE, d_name);
@@ -175,8 +175,7 @@
              outp->u.d_off = d_off;
              if ((sizeof (outp->u.d_ino) != sizeof (inp->k.d_ino)
                   && outp->u.d_ino != d_ino)
-                 || (sizeof (outp->u.d_off) != sizeof (inp->k.d_off)
-                     && outp->u.d_off != d_off))
+                 )
                {
                  /* Overflow.  If there was at least one entry
                     before this one, return them without error,
@@ -190,7 +189,10 @@
                  return -1;
                }
 
-             last_offset = d_off;
+             if( last_offset == -1 )
+               last_offset = 0;
+             last_offset += old_reclen;
+
              outp->u.d_reclen = new_reclen;
              outp->u.d_type = d_type;
 
@@ -213,6 +215,7 @@
     const size_t size_diff = (offsetof (DIRENT_TYPE, d_name)
                              - offsetof (struct kernel_dirent, d_name));
 
+    /* bug? (nbytes might be smaller than right side of minus) */
     red_nbytes = MIN (nbytes
                      - ((nbytes / (offsetof (DIRENT_TYPE, d_name) + 14))
                         * size_diff),