[v2] syscalls/shmctl05: new test for IPC file use-after-free bug

Message ID 20180710044649.13829-1-ebiggers3@gmail.com
State Superseded
Headers show
Series
  • [v2] syscalls/shmctl05: new test for IPC file use-after-free bug
Related show

Commit Message

Eric Biggers July 10, 2018, 4:46 a.m.
From: Eric Biggers <ebiggers@google.com>

Test for a bug in the System V IPC subsystem that resulted in a shared
memory file being used after it was freed (or being freed).

Signed-off-by: Eric Biggers <ebiggers@google.com>
---

Changed since v1:
    - Use using "fuzzy sync" spinlocks instead of random sleeps.
    - Use threads with .timeout instead of processes.
    - Use SAFE_SHMAT() instead of shmat() directly.
    - Use tst_timer_expired_ms() instead of
      tst_timer_stop() + tst_timer_elapsed_ms().

 runtest/syscalls                              |   1 +
 runtest/syscalls-ipc                          |   1 +
 .../kernel/syscalls/ipc/shmctl/.gitignore     |   1 +
 testcases/kernel/syscalls/ipc/shmctl/Makefile |   2 +
 .../kernel/syscalls/ipc/shmctl/shmctl05.c     | 112 ++++++++++++++++++
 5 files changed, 117 insertions(+)
 create mode 100644 testcases/kernel/syscalls/ipc/shmctl/shmctl05.c

Comments

Jan Stancek July 11, 2018, 7:57 a.m. | #1
----- Original Message -----
> From: Eric Biggers <ebiggers@google.com>
> 
> Test for a bug in the System V IPC subsystem that resulted in a shared
> memory file being used after it was freed (or being freed).
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Hi,

overall looks good to me, just 2 small comments inline.

> ---
> 
> Changed since v1:
>     - Use using "fuzzy sync" spinlocks instead of random sleeps.
>     - Use threads with .timeout instead of processes.
>     - Use SAFE_SHMAT() instead of shmat() directly.
>     - Use tst_timer_expired_ms() instead of
>       tst_timer_stop() + tst_timer_elapsed_ms().
> 
>  runtest/syscalls                              |   1 +
>  runtest/syscalls-ipc                          |   1 +
>  .../kernel/syscalls/ipc/shmctl/.gitignore     |   1 +
>  testcases/kernel/syscalls/ipc/shmctl/Makefile |   2 +
>  .../kernel/syscalls/ipc/shmctl/shmctl05.c     | 112 ++++++++++++++++++
>  5 files changed, 117 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/ipc/shmctl/shmctl05.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index a9afecf57..9eafd75d6 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1187,6 +1187,7 @@ shmctl01 shmctl01
>  shmctl02 shmctl02
>  shmctl03 shmctl03
>  shmctl04 shmctl04
> +shmctl05 shmctl05
>  
>  shmdt01 shmdt01
>  shmdt02 shmdt02
> diff --git a/runtest/syscalls-ipc b/runtest/syscalls-ipc
> index 00d7eed3a..54d8622d4 100644
> --- a/runtest/syscalls-ipc
> +++ b/runtest/syscalls-ipc
> @@ -53,6 +53,7 @@ shmctl01 shmctl01
>  shmctl02 shmctl02
>  shmctl03 shmctl03
>  shmctl04 shmctl04
> +shmctl05 shmctl05
>  
>  shmdt01 shmdt01
>  shmdt02 shmdt02
> diff --git a/testcases/kernel/syscalls/ipc/shmctl/.gitignore
> b/testcases/kernel/syscalls/ipc/shmctl/.gitignore
> index 9f5ac37ff..d6777e3b8 100644
> --- a/testcases/kernel/syscalls/ipc/shmctl/.gitignore
> +++ b/testcases/kernel/syscalls/ipc/shmctl/.gitignore
> @@ -2,3 +2,4 @@
>  /shmctl02
>  /shmctl03
>  /shmctl04
> +/shmctl05
> diff --git a/testcases/kernel/syscalls/ipc/shmctl/Makefile
> b/testcases/kernel/syscalls/ipc/shmctl/Makefile
> index f467389b9..ad5ea2507 100644
> --- a/testcases/kernel/syscalls/ipc/shmctl/Makefile
> +++ b/testcases/kernel/syscalls/ipc/shmctl/Makefile
> @@ -18,6 +18,8 @@
>  
>  top_srcdir              ?= ../../../../..
>  
> +shmctl05: LDLIBS += -lpthread

New code has been using:

CFLAGS += -pthread

to match more closely "Compile and link with -pthread".

> +
>  include $(top_srcdir)/include/mk/testcases.mk
>  include $(abs_srcdir)/../Makefile.inc
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/ipc/shmctl/shmctl05.c
> b/testcases/kernel/syscalls/ipc/shmctl/shmctl05.c
> new file mode 100644
> index 000000000..ae39ee382
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ipc/shmctl/shmctl05.c
> @@ -0,0 +1,112 @@
> +/*
> + * Copyright (c) 2018 Google, Inc.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program, if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * Regression test for commit 3f05317d9889 ("ipc/shm: fix use-after-free of
> shm
> + * file via remap_file_pages()").  This bug allowed the remap_file_pages()
> + * syscall to use the file of a System V shared memory segment after its ID
> had
> + * been reallocated and the file freed.  This test reproduces the bug as a
> NULL
> + * pointer dereference in touch_atime(), although it's a race condition so
> it's
> + * not guaranteed to work.  This test is based on the reproducer provided in
> the
> + * fix's commit message.
> + */
> +
> +#include "lapi/syscalls.h"
> +#include "tst_test.h"
> +#include "tst_fuzzy_sync.h"
> +#include "tst_safe_pthread.h"
> +#include "tst_safe_sysv_ipc.h"
> +#include "tst_timer.h"
> +
> +static struct tst_fzsync_pair fzsync_pair = TST_FZSYNC_PAIR_INIT;
> +
> +static pthread_t thrd;
> +
> +/*
> + * Thread 2: repeatedly remove the shm ID and reallocate it again for a
> + * new shm segment.
> + */
> +static void *thrproc(void *unused)
> +{
> +	int id = SAFE_SHMGET(0xF00F, 4096, IPC_CREAT|0700);
> +
> +	for (;;) {
> +		if (!tst_fzsync_wait_b(&fzsync_pair))
> +			break;
> +		SAFE_SHMCTL(id, IPC_RMID, NULL);
> +		id = SAFE_SHMGET(0xF00F, 4096, IPC_CREAT|0700);
> +		if (!tst_fzsync_wait_b(&fzsync_pair))
> +			break;
> +	}
> +	return unused;
> +}
> +
> +static void setup(void)
> +{
> +	/* Skip test if either remap_file_pages() or SysV IPC is unavailable */
> +	tst_syscall(__NR_remap_file_pages, NULL, 0, 0, 0, 0);
> +	tst_syscall(__NR_shmctl, 0xF00F, IPC_RMID, NULL);

There was a thread about something like "tst_timer_start_any()",
but that doesn't exist yet. So adding a check for specific
clock still seems to be needed in setup():

tst_timer_check(CLOCK_MONOTONIC);

Regards,
Jan

Patch

diff --git a/runtest/syscalls b/runtest/syscalls
index a9afecf57..9eafd75d6 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1187,6 +1187,7 @@  shmctl01 shmctl01
 shmctl02 shmctl02
 shmctl03 shmctl03
 shmctl04 shmctl04
+shmctl05 shmctl05
 
 shmdt01 shmdt01
 shmdt02 shmdt02
diff --git a/runtest/syscalls-ipc b/runtest/syscalls-ipc
index 00d7eed3a..54d8622d4 100644
--- a/runtest/syscalls-ipc
+++ b/runtest/syscalls-ipc
@@ -53,6 +53,7 @@  shmctl01 shmctl01
 shmctl02 shmctl02
 shmctl03 shmctl03
 shmctl04 shmctl04
+shmctl05 shmctl05
 
 shmdt01 shmdt01
 shmdt02 shmdt02
diff --git a/testcases/kernel/syscalls/ipc/shmctl/.gitignore b/testcases/kernel/syscalls/ipc/shmctl/.gitignore
index 9f5ac37ff..d6777e3b8 100644
--- a/testcases/kernel/syscalls/ipc/shmctl/.gitignore
+++ b/testcases/kernel/syscalls/ipc/shmctl/.gitignore
@@ -2,3 +2,4 @@ 
 /shmctl02
 /shmctl03
 /shmctl04
+/shmctl05
diff --git a/testcases/kernel/syscalls/ipc/shmctl/Makefile b/testcases/kernel/syscalls/ipc/shmctl/Makefile
index f467389b9..ad5ea2507 100644
--- a/testcases/kernel/syscalls/ipc/shmctl/Makefile
+++ b/testcases/kernel/syscalls/ipc/shmctl/Makefile
@@ -18,6 +18,8 @@ 
 
 top_srcdir              ?= ../../../../..
 
+shmctl05: LDLIBS += -lpthread
+
 include $(top_srcdir)/include/mk/testcases.mk
 include $(abs_srcdir)/../Makefile.inc
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/ipc/shmctl/shmctl05.c b/testcases/kernel/syscalls/ipc/shmctl/shmctl05.c
new file mode 100644
index 000000000..ae39ee382
--- /dev/null
+++ b/testcases/kernel/syscalls/ipc/shmctl/shmctl05.c
@@ -0,0 +1,112 @@ 
+/*
+ * Copyright (c) 2018 Google, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program, if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * Regression test for commit 3f05317d9889 ("ipc/shm: fix use-after-free of shm
+ * file via remap_file_pages()").  This bug allowed the remap_file_pages()
+ * syscall to use the file of a System V shared memory segment after its ID had
+ * been reallocated and the file freed.  This test reproduces the bug as a NULL
+ * pointer dereference in touch_atime(), although it's a race condition so it's
+ * not guaranteed to work.  This test is based on the reproducer provided in the
+ * fix's commit message.
+ */
+
+#include "lapi/syscalls.h"
+#include "tst_test.h"
+#include "tst_fuzzy_sync.h"
+#include "tst_safe_pthread.h"
+#include "tst_safe_sysv_ipc.h"
+#include "tst_timer.h"
+
+static struct tst_fzsync_pair fzsync_pair = TST_FZSYNC_PAIR_INIT;
+
+static pthread_t thrd;
+
+/*
+ * Thread 2: repeatedly remove the shm ID and reallocate it again for a
+ * new shm segment.
+ */
+static void *thrproc(void *unused)
+{
+	int id = SAFE_SHMGET(0xF00F, 4096, IPC_CREAT|0700);
+
+	for (;;) {
+		if (!tst_fzsync_wait_b(&fzsync_pair))
+			break;
+		SAFE_SHMCTL(id, IPC_RMID, NULL);
+		id = SAFE_SHMGET(0xF00F, 4096, IPC_CREAT|0700);
+		if (!tst_fzsync_wait_b(&fzsync_pair))
+			break;
+	}
+	return unused;
+}
+
+static void setup(void)
+{
+	/* Skip test if either remap_file_pages() or SysV IPC is unavailable */
+	tst_syscall(__NR_remap_file_pages, NULL, 0, 0, 0, 0);
+	tst_syscall(__NR_shmctl, 0xF00F, IPC_RMID, NULL);
+
+	SAFE_PTHREAD_CREATE(&thrd, NULL, thrproc, NULL);
+}
+
+static void do_test(void)
+{
+	tst_timer_start(CLOCK_MONOTONIC);
+
+	/*
+	 * Thread 1: repeatedly attach a shm segment, then remap it until the ID
+	 * seems to have been removed by the other process.
+	 */
+	while (!tst_timer_expired_ms(5000)) {
+		int id;
+		void *addr;
+
+		id = SAFE_SHMGET(0xF00F, 4096, IPC_CREAT|0700);
+		addr = SAFE_SHMAT(id, NULL, 0);
+		tst_fzsync_wait_a(&fzsync_pair);
+		do {
+			/* This is the system call that crashed */
+			TEST(syscall(__NR_remap_file_pages, addr, 4096,
+				     0, 0, 0));
+		} while (TEST_RETURN == 0);
+
+		if (TEST_ERRNO != EIDRM && TEST_ERRNO != EINVAL) {
+			tst_brk(TBROK | TTERRNO,
+				"Unexpected remap_file_pages() error");
+		}
+		tst_fzsync_wait_a(&fzsync_pair);
+	}
+
+	tst_res(TPASS, "didn't crash");
+}
+
+static void cleanup(void)
+{
+	if (thrd) {
+		tst_fzsync_pair_exit(&fzsync_pair);
+		SAFE_PTHREAD_JOIN(thrd, NULL);
+	}
+	shmctl(0xF00F, IPC_RMID, NULL);
+}
+
+static struct tst_test test = {
+	.timeout = 20,
+	.setup = setup,
+	.test_all = do_test,
+	.cleanup = cleanup,
+};