Created attachment 437184 [details] test program to demonstrate problem Description of problem: When running a test program to check the buffer contents returned by readdir_r there appears to be data returned beyond the end of the dirent structure. The test program allocates extra buffer space and fills it with a known value for comparison. Version-Release number of selected component (if applicable): kernel-2.6.18-194.el5 How reproducible: always Steps to Reproduce: 1. gcc -g -o0 -o readdir_r_buf_overrun readdir_r_buf_overrun.c 2. mkdir run; cd run 3. ../readdir_r_buf_overrun I also ran this via gdb to examine the full contents of the dirent structure and the buffer after return from readdir_r, i.e., gcc -g -o0 -o readdir_r_buf_overrun readdir_r_buf_overrun.c gdb readdir_r_buf_overrun break 45 set print elements 300 run print *buf x/272xb buf Actual results: dentry: long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-filename buf overrun check: xooooooooooooooo BUFFER OVERRUN DETECTED !! (gdb) x/272xb buf 0x8052028: 0xbe 0x88 0xf8 0x03 0x03 0x00 0x00 0x00 : 0x8052120: 0x6e 0x67 0x2d 0x6c 0x6f 0x6e 0x67 0x2d 0x8052128: 0x66 0x69 0x6c 0x65 0x6e 0x61 0x6d 0x65 0x8052130: 0x00 0x00 0x00 0x08 0x63 0x63 0x63 0x63 ^^^^ ^^^^ ^^^^ Expected results: No overrun. : 0x8052128: 0x66 0x69 0x6c 0x65 0x6e 0x61 0x6d 0x65 0x8052130: 0x00 0x63 0x63 0x63 0x63 0x63 0x63 0x63 ^^^^ ^^^^ ^^^^ Additional info: I found that the extra data also appears in 'some' reads of shorter file/directory names, i.e., (gdb) print *buf $4 = {d_ino = 33216788, d_off = 5, d_reclen = 36, d_type = 8 '\b', d_name = "readdir_r_buf_overrun.c\000\b", 'c' <repeats 231 times>} (gdb) x/272xb buf 0x8052028: 0x14 0xd9 0xfa 0x01 0x05 0x00 0x00 0x00 0x8052030: 0x24 0x00 0x08 0x72 0x65 0x61 0x64 0x64 0x8052038: 0x69 0x72 0x5f 0x72 0x5f 0x62 0x75 0x66 0x8052040: 0x5f 0x6f 0x76 0x65 0x72 0x72 0x75 0x6e 0x8052048: 0x2e 0x63 0x00 0x08 0x63 0x63 0x63 0x63 : ^^^^ but this does not overflow the allocated space in d_name. In linux-2.6.18.i686/fs/readdir.c I saw: #define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de))) #define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1)) : static int filldir(void * __buf, const char * name, int namlen, loff_t offset, u64 ino, unsigned int d_type) { struct linux_dirent __user * dirent; struct getdents_callback * buf = (struct getdents_callback *) __buf; unsigned long d_ino; int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2); Could the reclen calculation be causing extra bytes to be returned to userspace? If so, might this overrun the allocated space for dirent in the event of very long file names? note: This is also reproducible on other RHEL5 releases, and recent Fedora versions (e.g., F12).
The 0x08 you see is d_type: "The d_type field is implemented since Linux 2.6.4. It occupies a space that was previously a zero-filled padding byte in the linux_dirent structure. Thus, on kernels before 2.6.3, attempting to access this field always provides the value 0 (DT_UNKNOWN)." You got 0x08 /* * File types * * NOTE! These match bits 12..15 of stat.st_mode * (ie "(i_mode >> 12) & 15"). */ ... #define DT_REG 8 I'm not certain about why you see more nulls before it, I'll have to look into that rounding up a little more. How did you find this, did you find any actual functional problem? Thanks, -Eric
I'm not sure I made one point clear. I see the dirent structure looks like: struct dirent { #ifndef __USE_FILE_OFFSET64 __ino_t d_ino; __off_t d_off; #else __ino64_t d_ino; __off64_t d_off; #endif unsigned short int d_reclen; unsigned char d_type; char d_name[256]; /* We must not include limits.h! */ }; and that d_type is the forth element in the structure, just before d_name, but the issue I'm seeing is that data may be returned _after_ d_name, and it (coincidentally?) matches the d_type value. If I'm looking at this correctly there shouldn't be any data returned after the string in d_name. In the second (short name) example I posted you can see this more clearly: ---------------------- (gdb) print *buf $4 = {d_ino = 33216788, d_off = 5, d_reclen = 36, d_type = 8 '\b', d_name = "readdir_r_buf_overrun.c\000\b", 'c' <repeats 231 times>} ^^^ (gdb) x/272xb buf 0x8052028: 0x14 0xd9 0xfa 0x01 0x05 0x00 0x00 0x00 { d_ino } { d_off } 0x8052030: 0x24 0x00 0x08 0x72 0x65 0x61 0x64 0x64 { d_reclen } {d_type} { d_name ... 0x8052038: 0x69 0x72 0x5f 0x72 0x5f 0x62 0x75 0x66 0x8052040: 0x5f 0x6f 0x76 0x65 0x72 0x72 0x75 0x6e 0x8052048: 0x2e 0x63 0x00 0x08 0x63 0x63 0x63 0x63 ... } {Null} {????} : ^^^^ ---------------------- Should that last 0x08 be there (in the buffer _after_ the null terminated string)? If so, what puts it there and why? On the first (long name) example I posted, the extra data after d_name would overflow the buffer length, if the extra (16) bytes had not been allocated. On shorter names it would probably go unnoticed, since it is within the buffer space and after the null terminator. I hope this clarifies the issue I'm seeing. This issue was reported by a customer using a custom kernel that we provided, which is based on a Fedora 10 kernel, but I found it was also reproducible on the stock RHEL5 kernel so I reported it here. Since it was still reproducible on F12 it may affect RHEL6 as well.
Hello everyone. I confirm the overflow. It occurs in glibc function readdir_r (3). So in accordance with POSIX recommendations user allocates a buffer of length offsetof(struct dirent, d_name) + pathconf(dirpath, _PC_NAME_MAX) + 1 (*) and passes it to readdir_r(). The last one reads directory entries to the directory stream using getdents (2) system call, converts kernel format of directory entry (struct kernel_dirent) to the POSIX format (struct dirent) by function __GETDENTS() and passes a directory entry in POSIX format to user. The overflow occurs in line 116, file sysdeps/unix/readdir_r.c, function __READDIR_R (glibc-2.5-42): *result = memcpy (entry, dp, reclen); here @reclen is size of directory entry in kernel format, which is calculated and passed by kernel. However, directory entry in kernel format is *larger* than directory entry in POSIX format: kernel rounds size of his format up by sizeof(long), but POSIX doesn't have such recommendations (AFAIK). In our testcase reclen = 268 (253 + 1 + 11 = 265, rounded up by 4), and user allocates buffer of smaller size (*) 255 + 11 + 1 = 267, so we get problems every time when size of directory entry name (without null-termination) is 253, 254 or 255. So I wouldn't blame a new kernel getdents interface: everything is okay here, moreover glibc uses it correctly. Also I wouldn't blame users, as they follow to POSIX recommendations (however, the problem will disappear, if user allocates buffer of size, which is rounded up by sizeof long). I think that glibc is the culprit: why do we need to make an assumption that POSIX and kernel formats have identical sizes? glibc maintainer cc-ed, please, take a look. Thanks, Edward.
Has there been any more progress on this issue?
The customer is asking about this again, so I wanted to check if there was any progress to report. Will someone please provide a status update? Thank you.
Please help - the customer is asking for an update. Has a glibc maintainer looked at this issue as requested in Comment #3? Thanks, Beth
The storage pointed to by entry shall be large enough for a dirent with an array of char d_name members containing at least {NAME_MAX}+1 elements. The array of char d_name is not a fixed size. Implementations may need to declare struct dirent with an array size for d_name of 1, but the actual number of characters provided matches (or only slightly exceeds) the length of the filename.
From your reply: > Implementations may need to declare struct dirent with an array size for d_name of 1, but the actual number of characters provided matches (or only slightly exceeds) the length of the filename. but as far as I can tell they were following the recommendations for the amount of space to allocate, i.e., Since POSIX.1 does not specify the size of the d_name field, and other nonstandard fields may precede that field within the dirent structure, portable applications that use readdir_r() should allocate the buffer whose address is passed in entry as follows: len = offsetof(struct dirent, d_name) + pathconf(dirpath, _PC_NAME_MAX) + 1 entryp = malloc(len); Is a different calculation recommended to ensure adequate space is allocated for the d_name entry? If so, will you please provide a pointer to the description in the documentation so we can give that to the customer for reference?
I my opinion the line: reclen = MIN (reclen, NAME_MAX); should be inserted before line: *result = memcpy (entry, dp, reclen); In function __READDIR_R () in file /sysdeps/unix/readdir_r.c Reasoning is According to Posix docs from http://pubs.opengroup.org/onlinepubs/009695399/basedefs/dirent.h.html "The character array d_name is of unspecified size, but the number of bytes preceding the terminating null byte shall not exceed {NAME_MAX}." in same doc under Application Usage: "The name of an array of char of an unspecified size should not be used as an lvalue. Use of: sizeof(d_name) is incorrect; use: strlen(d_name) instead." and from http://pubs.opengroup.org/onlinepubs/009695399/functions/readdir.html "The storage pointed to by entry shall be large enough for a dirent with an array of char d_name members containing at least {NAME_MAX}+1 elements." To me it says that if user allocates NAME_MAX + 1 to d_name they can assume they are safe, which isn't the case currently in Linux as proven by the case in thiz BZ. I can understand why glibc would not use strlen as that is time consuming but I think it should never copy more than NAME_MAX characters.
Ulrich Drepper informed me on 2010-12-19 that he checked in a patch upstream for this issue.
You must never pass less than struct dirent.
My suggested fix in https://bugzilla.redhat.com/show_bug.cgi?id=621953#c9 was incorrect. It should be: reclen = MIN (reclen, sizeof(struct dirent)); be inserted before line: *result = memcpy (entry, dp, reclen); there is probably better way of insuring reclen is not larger than maximum possible set by POSIX. I believe Uli submitted a patch to do this, I haven't confirmed. In the version of readdir_r.c I have (dated 2010-02-22), on line 90 reclen = dp->d_reclen; dp->d_reclen comes from GETDENTS call on line 64 bytes = __GETDENTS (dirp->fd, dirp->data, maxread); According to Edward Shishkin in https://bugzilla.redhat.com/show_bug.cgi?id=621953#c3 The kernel's GETDENTS call can set the reclen to larger than maximum size allowed for struct dirent by POSIX standards due to rounding up the size to nearest size of long. simple example with old simple dirent the max size is: unsigned short int d_reclen; (2 bytes) unsigned char d_type; (1 byte) char d_name[NAME_MAX+1]; (256 bytes) total (259 bytes) with this simple example GETDENTS would allocate 260 bytes and return d_reclen value of 260 when pathname was NAME_MAX long. readdir_r.c would then copy 260 bytes into a dirent structure passed in by user with direct struct of 259 bytes in line 117 *result = memcpy (entry, dp, reclen); causing overrun of the entry dirent struct.
Here is a link to Uli's patch for this issue: http://sourceware.org/git/?p=glibc.git;a=commitdiff;h=1a81139728494810f65aaa0d0c538ff8c2783dd5#patch1
The test program is simply INVALID. The buffer size is smaller than struct dirent.
In my testing on x86_64, the allocated buffer is 291 bytes, while the size of struct dirent is 280. What am I missing? bufsize = offsetof(struct dirent, d_name) + name_max + 1; if ((buf = (struct dirent *)malloc(bufsize + BUF_EXTRA)) == NULL) { perror("malloc"); } printf("bufsize %d allocated %d dirent %d\n", bufsize, bufsize + BUF_EXTRA, sizeof(struct dirent)); bufsize 275 allocated 291 dirent 280
Since this is not considered a bug, but fails when using the suggested code from the man page, I am switching this BZ to the man-page component and requesting the following text: Since POSIX.1 does not specify the size of the d_name field, and other nonstandard fields may precede that field within the dirent structure, portable applications that use readdir_r() should allocate the buffer whose address is passed in entry as follows: len = offsetof(struct dirent, d_name) + pathconf(dirpath, _PC_NAME_MAX) + 1 entryp = malloc(len); (POSIX.1 requires that d_name is the last field in a struct dirent.) be changed to recommend: : len = sizeof(struct dirent); entryp = malloc(len); : This appears to work for some test cases tried, but I wanted to confirm that this is the correct change, and if so, have it applied to the man page.
This request was evaluated by Red Hat Product Management for inclusion in the current release of Red Hat Enterprise Linux. Because the affected component is not scheduled to be updated in the current release, Red Hat is unfortunately unable to address this request at this time. Red Hat invites you to ask your support representative to propose this request, if appropriate and relevant, in the next release of Red Hat Enterprise Linux.
Please, can somebody confirm that comment #17 is correct, so I can apply it to the man page? Thanks.
Eric, in response to c#16: sizeof (anything) is going to be rounded up to its natural boundary which will necessitate padding. This is vital so that allocating and iterating on an array of these structures will work properly. I don't think the change requested in c#17 is correct. What really needs to happen is to account for the rounding/tail padding. AFAICT, this has been fixed as man readdir_r shows the following on my F16 system: Since POSIX.1 does not specify the size of the d_name field, and other nonstandard fields may precede that field within the dirent structure, portable applications that use read‐ dir_r() should allocate the buffer whose address is passed in entry as follows: len = (offsetof(struct dirent, d_name) + pathconf(dirpath, _PC_NAME_MAX) + 1 + sizeof(long)) & -sizeof(long); entryp = malloc(len); Note the "(... + sizeof (long)) & -sizeof(long) That will account for the rounding.
Created attachment 587263 [details] readdir.patch Patch adding the note mentioned in the comment 20. Is this patch sufficient to fix this bug?
This request was evaluated by Red Hat Product Management for inclusion in a Red Hat Enterprise Linux release. Product Management has requested further review of this request by Red Hat Engineering, for potential inclusion in a Red Hat Enterprise Linux release for currently deployed products. This request is not yet committed for inclusion in a release.
Created attachment 598637 [details] readdir.patch
Created attachment 598670 [details] readdir.patch one last update of attached patch - I've back-ported readdir.3 man page from RHEL-6 as it contains much more useful information
Technical note added. If any revisions are required, please edit the "Technical Notes" field accordingly. All revisions will be proofread by the Engineering Content Services team. New Contents: Cause: undocumented size of the buffer "entry" for the readdir_r function in the readdir.3 manual page Consequence: possible overflow in user programs Fix: back-port of the readdir.3 manual page from RHEL-6 Result: complete documentation of readdir and readdir_r functions
Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. http://rhn.redhat.com/errata/RHBA-2013-0073.html