On Mon, Aug 04, 2014 at 07:42:57AM +0530, Zubin Mithra wrote:
> 
> * file.c (sys_getdents): Add d_reclen check.
> (sys_getdents64): Add d_reclen check.
> 
> Signed-off-by: Zubin Mithra <zubin.mithra@gmail.com>
> ---
>  file.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/file.c b/file.c
> index a92a7dc..0934ce1 100644
> --- a/file.c
> +++ b/file.c
> @@ -2076,6 +2076,10 @@ sys_getdents(struct tcb *tcp)
>  				i ? " " : "", d->d_ino, d->d_off);
>  			tprintf("d_reclen=%u, d_name=\"%s\", d_type=",
>  				d->d_reclen, d->d_name);
> +			if (i + d->d_reclen >= len) {
> +				tprints("...}");
> +				break;
> +			}
>  			printxval(direnttypes, buf[i + d->d_reclen - 1], "DT_???");
>  			tprints("}");
>  		}

I was talking about this d_reclen check back in April, but this is not the
only out-of-bounds read issue with getdents.

> @@ -2138,8 +2142,13 @@ sys_getdents64(struct tcb *tcp)
>  			tprints("d_type=");
>  			printxval(direnttypes, d->d_type, "DT_???");
>  			tprints(", ");
> -			tprintf("d_reclen=%u, d_name=\"%s\"}",
> +			tprintf("d_reclen=%u, d_name=\"%s\"",
>  				d->d_reclen, d->d_name);
> +			if (i + d->d_reclen >= len) {
> +				tprints("...}");
> +				break;
> +			}
> +			tprints("}");
>  		}
>  		if (!d->d_reclen) {
>  			tprints("/* d_reclen == 0, problem here */");

getdents64 doesn't need this check, but there are other out-of-bounds read
issues similar to getdents.

Here is a fix of potential out-of-bounds read issues in
getdents/getdents64 I was thinking of:

diff --git a/file.c b/file.c
index 986f446..0044429 100644
--- a/file.c
+++ b/file.c
@@ -2060,7 +2060,7 @@ sys_readdir(struct tcb *tcp)
 int
 sys_getdents(struct tcb *tcp)
 {
-	int i, len, dents = 0;
+	unsigned int i, len, dents = 0;
 	char *buf;
 
 	if (entering(tcp)) {
@@ -2072,38 +2072,55 @@ sys_getdents(struct tcb *tcp)
 		tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]);
 		return 0;
 	}
-	len = tcp->u_rval;
-	/* Beware of insanely large or negative values in tcp->u_rval */
+
+	/* Beware of insanely large or too small values in tcp->u_rval */
 	if (tcp->u_rval > 1024*1024)
 		len = 1024*1024;
-	if (tcp->u_rval < 0)
+	else if (tcp->u_rval < (int) sizeof(struct kernel_dirent))
 		len = 0;
-	buf = len ? malloc(len) : NULL;
-	if (len && !buf)
-		die_out_of_memory();
-	if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) {
-		tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]);
-		free(buf);
-		return 0;
+	else
+		len = tcp->u_rval;
+
+	if (len) {
+		buf = malloc(len);
+		if (!buf)
+			die_out_of_memory();
+		if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) {
+			tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]);
+			free(buf);
+			return 0;
+		}
+	} else {
+		buf = NULL;
 	}
+
 	if (!abbrev(tcp))
 		tprints("{");
-	for (i = 0; i < len;) {
+	for (i = 0; len && i <= len - sizeof(struct kernel_dirent); ) {
 		struct kernel_dirent *d = (struct kernel_dirent *) &buf[i];
+
 		if (!abbrev(tcp)) {
+			int oob = d->d_reclen < sizeof(struct kernel_dirent) ||
+				  i + d->d_reclen - 1 >= len;
+			int d_name_len = oob ? len - i : d->d_reclen;
+			d_name_len -= offsetof(struct kernel_dirent, d_name) + 1;
+
 			tprintf("%s{d_ino=%lu, d_off=%lu, ",
 				i ? " " : "", d->d_ino, d->d_off);
-			tprintf("d_reclen=%u, d_name=\"%s\", d_type=",
-				d->d_reclen, d->d_name);
-			printxval(direnttypes, buf[i + d->d_reclen - 1], "DT_???");
+			tprintf("d_reclen=%u, d_name=\"%.*s\", d_type=",
+				d->d_reclen, d_name_len, d->d_name);
+			if (oob)
+				tprints("?");
+			else
+				printxval(direnttypes, buf[i + d->d_reclen - 1], "DT_???");
 			tprints("}");
 		}
-		if (!d->d_reclen) {
-			tprints("/* d_reclen == 0, problem here */");
+		dents++;
+		if (d->d_reclen < sizeof(struct kernel_dirent)) {
+			tprints("/* d_reclen < sizeof(struct kernel_dirent) */");
 			break;
 		}
 		i += d->d_reclen;
-		dents++;
 	}
 	if (!abbrev(tcp))
 		tprints("}");
@@ -2117,7 +2134,10 @@ sys_getdents(struct tcb *tcp)
 int
 sys_getdents64(struct tcb *tcp)
 {
-	int i, len, dents = 0;
+	/* the minimum size of a valid dirent64 structure */
+	const unsigned int d_name_offset = offsetof(struct dirent64, d_name);
+
+	unsigned int i, len, dents = 0;
 	char *buf;
 
 	if (entering(tcp)) {
@@ -2130,38 +2150,52 @@ sys_getdents64(struct tcb *tcp)
 		return 0;
 	}
 
-	len = tcp->u_rval;
-	/* Beware of insanely large or negative tcp->u_rval */
+	/* Beware of insanely large or too small tcp->u_rval */
 	if (tcp->u_rval > 1024*1024)
 		len = 1024*1024;
-	if (tcp->u_rval < 0)
+	else if (tcp->u_rval < (int) d_name_offset)
 		len = 0;
-	buf = len ? malloc(len) : NULL;
-	if (len && !buf)
-		die_out_of_memory();
-
-	if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) {
-		tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]);
-		free(buf);
-		return 0;
+	else
+		len = tcp->u_rval;
+
+	if (len) {
+		buf = malloc(len);
+		if (!buf)
+			die_out_of_memory();
+		if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) {
+			tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]);
+			free(buf);
+			return 0;
+		}
+	} else {
+		buf = NULL;
 	}
+
 	if (!abbrev(tcp))
 		tprints("{");
-	for (i = 0; i < len;) {
+	for (i = 0; len && i <= len - d_name_offset; ) {
 		struct dirent64 *d = (struct dirent64 *) &buf[i];
 		if (!abbrev(tcp)) {
-			tprintf("%s{d_ino=%" PRIu64 ", d_off=%" PRId64 ", ",
+			int d_name_len;
+			if (d->d_reclen >= d_name_offset
+			    && i + d->d_reclen <= len) {
+				d_name_len = d->d_reclen - d_name_offset;
+			} else {
+				d_name_len = len - i - d_name_offset;
+			}
+
+			tprintf("%s{d_ino=%" PRIu64 ", d_off=%" PRId64
+				", d_reclen=%u, d_type=",
 				i ? " " : "",
 				d->d_ino,
-				d->d_off);
-			tprints("d_type=");
+				d->d_off,
+				d->d_reclen);
 			printxval(direnttypes, d->d_type, "DT_???");
-			tprints(", ");
-			tprintf("d_reclen=%u, d_name=\"%s\"}",
-				d->d_reclen, d->d_name);
+			tprintf(", d_name=\"%.*s\"}",
+				d_name_len, d->d_name);
 		}
-		if (!d->d_reclen) {
-			tprints("/* d_reclen == 0, problem here */");
+		if (d->d_reclen < d_name_offset) {
+			tprints("/* d_reclen < offsetof(struct dirent64, d_name) */");
 			break;
 		}
 		i += d->d_reclen;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 922452a..8f22d6d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -20,6 +20,7 @@ TESTS = \
 	qual_syscall.test \
 	scm_rights-fd.test \
 	sigaction.test \
+	getdents.test \
 	stat.test \
 	net.test \
 	net-fd.test \
diff --git a/tests/getdents.test b/tests/getdents.test
new file mode 100755
index 0000000..187f04e
--- /dev/null
+++ b/tests/getdents.test
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+# Check that getdents/getdents64 syscalls are traced properly.
+
+. "${srcdir=.}/init.sh"
+
+check_prog grep
+check_prog ls
+check_prog mkdir
+check_prog rmdir
+
+mkdir emptydir ||
+	framework_skip_ 'failed to create an empty directory'
+
+ls emptydir ||
+	{ rmdir emptydir; framework_skip_ 'failed to list an empty directory'; }
+
+args='-vegetdents,getdents64'
+dirent='\{d_ino=[0-9]+, d_off=[0-9]+, d_reclen=[1-9][0-9]*, d_name="\.\.?", d_type=DT_DIR\}'
+dirent64='\{d_ino=[0-9]+, d_off=[0-9]+, d_reclen=[1-9][0-9]*, d_type=DT_DIR, d_name="\.\.?"\}'
+dirent_or_dirent64="($dirent $dirent|$dirent64 $dirent64)"
+getdents1="getdents(64)?\([0-9]+, \{$dirent_or_dirent64\}, [1-9][0-9]*\) += [1-9][0-9]*"
+getdents2='getdents(64)?\([0-9]+, \{\}, [1-9][0-9]*\) += 0'
+
+$STRACE $args -o $LOG ls emptydir &&
+LC_ALL=C grep -E -x "$getdents1" $LOG > /dev/null &&
+LC_ALL=C grep -E -x "$getdents2" $LOG > /dev/null ||
+	{ rmdir emptydir; cat $LOG; fail_ "strace $args failed to trace getdents/getdents64 properly"; }
+
+rmdir emptydir
+
+exit 0


-- 
ldv