[2/2] readdir02: use invalid DIR stream descriptor
diff mbox series

Message ID 20181220090811.21514-2-liwang@redhat.com
State Changes Requested
Headers show
Series
  • [1/2] readdir: rewrite readdir02
Related show

Commit Message

Li Wang Dec. 20, 2018, 9:08 a.m. UTC
Issue:
  On ppc64le and aarch64, when testing in NFS mountpoint, test
  process receives SIGSEGV when calling readdir on a DIR which
  has just been closed by closedir().

  Unfortunately, ltp/readdir02.c handles SIGSEGV. This makes it
  hits SIGSEGV again in its cleanup function. So readdir02 hangs
  there hitting SEGV endlessly.

That's because a DIR * is NOT a file descriptor. It's memory
allocated by opendir() that contains libc internal information
about the directory. closedir(test_dir) frees any memory associated
with the open directory pointer test_dir.

To then pass the freed dir pointer to readdir() is a use-after-free.
It probably won't return EBADF, it will dereference freed memory
and whatever happens after that is undefined.

In this patch, I simply modify the test to use an exist FILE *
stream to simulate the invalid directory stream descriptor. Then
it won't hit the use-after-free issue any more.

Also, the sighandler function has been dropped.

Reported-by: Xiong Zhou <xzhou@redhat.com>
Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Scott Mayhew <smayhew@redhat.com>
---
 testcases/kernel/syscalls/readdir/readdir02.c | 64 +++++++++++----------------
 1 file changed, 25 insertions(+), 39 deletions(-)

Comments

Cyril Hrubis Jan. 28, 2019, 3:16 p.m. UTC | #1
Hi!
> Issue:
>   On ppc64le and aarch64, when testing in NFS mountpoint, test
>   process receives SIGSEGV when calling readdir on a DIR which
>   has just been closed by closedir().
> 
>   Unfortunately, ltp/readdir02.c handles SIGSEGV. This makes it
>   hits SIGSEGV again in its cleanup function. So readdir02 hangs
>   there hitting SEGV endlessly.
> 
> That's because a DIR * is NOT a file descriptor. It's memory
> allocated by opendir() that contains libc internal information
> about the directory. closedir(test_dir) frees any memory associated
> with the open directory pointer test_dir.
> 
> To then pass the freed dir pointer to readdir() is a use-after-free.
> It probably won't return EBADF, it will dereference freed memory
> and whatever happens after that is undefined.
> 
> In this patch, I simply modify the test to use an exist FILE *
> stream to simulate the invalid directory stream descriptor. Then
> it won't hit the use-after-free issue any more.

Actually I think that the best we can do here is to delete the testcase
because:

* Casting FILE* to DIR* is IMHO invoking even worse undefined behavior
  than the original test that called readdir() on closed DIR*

* We do cover the EBADF for getents() syscalls getents02 test
Li Wang Feb. 1, 2019, 6:59 a.m. UTC | #2
On Mon, Jan 28, 2019 at 11:19 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> >
> > In this patch, I simply modify the test to use an exist FILE *
> > stream to simulate the invalid directory stream descriptor. Then
> > it won't hit the use-after-free issue any more.
>
> Actually I think that the best we can do here is to delete the testcase
> because:
>
> * Casting FILE* to DIR* is IMHO invoking even worse undefined behavior
>   than the original test that called readdir() on closed DIR*
>

Why say this? Does this CASTING will do something more bad? AFAICT that
changing an variable of one data type into another, and the worst
harmness is to loss of information in the variable so we'd better avoid
that. But in this test we only need a invalid DIR* for readdir() tesst, it
does *not* really care about the pointer content I guess?



>
> * We do cover the EBADF for getents() syscalls getents02 test
>

I'm sorry, I don't find this testcase in LTP, or did I miss anything?
Cyril Hrubis Feb. 7, 2019, 12:51 p.m. UTC | #3
Hi!
> > > In this patch, I simply modify the test to use an exist FILE *
> > > stream to simulate the invalid directory stream descriptor. Then
> > > it won't hit the use-after-free issue any more.
> >
> > Actually I think that the best we can do here is to delete the testcase
> > because:
> >
> > * Casting FILE* to DIR* is IMHO invoking even worse undefined behavior
> >   than the original test that called readdir() on closed DIR*
> >
> 
> Why say this? Does this CASTING will do something more bad?

Yes.

> AFAICT that changing an variable of one data type into another, and
> the worst harmness is to loss of information in the variable so we'd
> better avoid that. But in this test we only need a invalid DIR* for
> readdir() tesst, it does *not* really care about the pointer content I
> guess?

Not at all, both FILE and DIR are typedefs to C structures, which are
just chunks of memory, by doing this you are basically passing random
data to the call because all it does when the C library gets the fd from
these strucutres is that it takes bytes from at some offest in the chunk
of memory. There are no abstract types, methods or objects in C, just
chunks of memory.

> >
> > * We do cover the EBADF for getents() syscalls getents02 test
> >
> 
> I'm sorry, I don't find this testcase in LTP, or did I miss anything?

Sorry typo, it's getdents02.
Li Wang Feb. 15, 2019, 8:40 a.m. UTC | #4
Hi Cyril,

Sorry for late reply, since it was China holidays in past two weeks.

On Thu, Feb 7, 2019 at 8:51 PM Cyril Hrubis <chrubis@suse.cz> wrote:

>
> > AFAICT that changing an variable of one data type into another, and
> > the worst harmness is to loss of information in the variable so we'd
> > better avoid that. But in this test we only need a invalid DIR* for
> > readdir() tesst, it does *not* really care about the pointer content I
> > guess?
>
> Not at all, both FILE and DIR are typedefs to C structures, which are
> just chunks of memory, by doing this you are basically passing random
> data to the call because all it does when the C library gets the fd from
> these strucutres is that it takes bytes from at some offest in the chunk
> of memory. There are no abstract types, methods or objects in C, just
> chunks of memory.
>
>
Ok, I got the point, thanks for the explanation. I will help to rewrite a
new patch for readdir02 deleting.

Patch
diff mbox series

diff --git a/testcases/kernel/syscalls/readdir/readdir02.c b/testcases/kernel/syscalls/readdir/readdir02.c
index 441c4b431..21d00cb0a 100644
--- a/testcases/kernel/syscalls/readdir/readdir02.c
+++ b/testcases/kernel/syscalls/readdir/readdir02.c
@@ -36,59 +36,45 @@ 
 #include <signal.h>
 
 #include "tst_test.h"
+#include "tst_safe_stdio.h"
+
+#define TEST_FILE "readdir_file.txt"
 
 static void verify_readdir(void)
 {
+	FILE *fp;
 	DIR *test_dir;
 	struct dirent *dptr;
 
-	if ((test_dir = opendir(".")) == NULL) {
-		tst_res(TFAIL, "opendir(\".\") Failed, errno=%d : %s",
-			 errno, strerror(errno));
-	} else {
-		if (closedir(test_dir) < 0) {
+	fp = SAFE_FOPEN(TEST_FILE, "ab+");
+	/* regard FILE * as an invalid directory stream descriptor */
+	test_dir = (DIR *)fp;
+
+	dptr = readdir(test_dir);
+	switch (errno) {
+	case EBADF:
+		tst_res(TPASS,
+			"expected failure - errno = %d : %s",
+			errno, strerror(errno));
+		break;
+	default:
+		if (dptr != NULL) {
 			tst_res(TFAIL,
-				 "closedir(\".\") Failed, errno=%d : %s",
-				 errno, strerror(errno));
+				"call failed with an "
+				"unexpected error - %d : %s",
+				errno,
+				strerror(errno));
 		} else {
-			dptr = readdir(test_dir);
-			switch (errno) {
-			case EBADF:
-				tst_res(TPASS,
-					 "expected failure - errno = %d : %s",
-					 errno, strerror(errno));
-				break;
-			default:
-				if (dptr != NULL) {
-					tst_brk(TFAIL,
-						 "call failed with an "
-						 "unexpected error - %d : %s",
-						 errno,
-						 strerror(errno));
-				} else {
-					tst_res(TINFO,
-						 "readdir() is not _required_ to fail, "
-						 "errno = %d  ", errno);
-				}
-			}
+			tst_res(TINFO,
+				"readdir() is not _required_ to fail, "
+				"errno = %d ", errno);
 		}
 	}
-}
 
-static void sighandler(int sig LTP_ATTRIBUTE_UNUSED)
-{
-	tst_res(TCONF,
-		 "This system's implementation of closedir() "
-		 "will not allow this test to execute properly.");
-}
-
-static void setup(void)
-{
-	SAFE_SIGNAL(SIGSEGV, sighandler);
+	SAFE_FCLOSE(fp);
 }
 
 static struct tst_test test = {
 	.needs_tmpdir = 1,
-	.setup = setup,
 	.test_all = verify_readdir,
 };