[LTP] [PATCH v5 0/3] Add regression test for CVE-2017-17053

[LTP] [PATCH v5 0/3] Add regression test for CVE-2017-17053

[Michael Moese]

Add a regression test for CVE-2017-17053. This testcase is depending
on some new library functions included in this series.

This patch series consists of reworked patches according to previous
review comments, as well as a small new library wrapper function
SAFE_SIGACTION() to install a signal handler.

This v5 series contains changes according to suggestions after review.


Michael Moese (3):
  Add library support for /proc/sys/kernel/tainted
  Add a library wrapper for sigaction()
  Add regression test for CVE-2017-17053

 doc/test-writing-guidelines.txt |  42 ++++++++++
 include/tst_safe_macros.h       |  20 +++++
 include/tst_taint.h             | 104 ++++++++++++++++++++++++
 lib/tst_taint.c                 | 106 +++++++++++++++++++++++++
 runtest/cve                     |   1 +
 testcases/cve/.gitignore        |   1 +
 testcases/cve/Makefile          |   1 +
 testcases/cve/cve-2017-17053.c  | 171 ++++++++++++++++++++++++++++++++++++++++
 8 files changed, 446 insertions(+)
 create mode 100644 include/tst_taint.h
 create mode 100644 lib/tst_taint.c
 create mode 100644 testcases/cve/cve-2017-17053.c

-- 
2.13.6

[LTP] [PATCH v5 1/3] Add library support for /proc/sys/kernel/tainted

[Michael Moese]

Sometimes, it is important to detect if the kernel has issued a
warning, died, or is tainted in another way. Linux provides this
information in /proc/sys/kernel/tainted in the form of a bitfield.
This patch provides library functions for testcases to detect, if
it has tainted the kernel.

The following functions will be introduced:

- int tst_taint_init(unsigned int mask)
  check if the flags supplied as mask are supported by the running
  kernel, and if so, if they are not yet set.

- int tst_taint_check()
  check if one or more of the bits specified in the mask provided
  to tst_taint_init() before are set.
  Returns 0 if those flags are not set, or the bitmask of set flags

These can be used in the following way:

First, during testcase setup:

void setup(void)
{
	...
	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
}

Second, check if the test triggered a bug:

void run(void)
{
	...
	. test code here
	...
	if (tst_taint_check() != 0)
		tst_res(TFAIL, "kernel has issues");
	 else
		tst_res(TPASS, "kernel seems to be fine");
}

Signed-off-by: Michael Moese <mmoese@suse.de>
---
 doc/test-writing-guidelines.txt |  42 ++++++++++++++++
 include/tst_taint.h             | 104 +++++++++++++++++++++++++++++++++++++++
 lib/tst_taint.c                 | 106 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 252 insertions(+)
 create mode 100644 include/tst_taint.h
 create mode 100644 lib/tst_taint.c

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 739b295b8..f12386485 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1312,6 +1312,48 @@ common.h:9: FAIL: check failed
 test.c:8: INFO: do_action(arg) failed
 -------------------------------------------------------------------------------
 
+2.2.24 Tainted kernels
+^^^^^^^^^^^^^^^^^^^^^^
+
+If you need to detect, if a testcase triggers a kernel warning, bug or oops,
+the following can be used to detect TAINT_W or TAINT_D:
+
+[source,c]
+-------------------------------------------------------------------------------
+#include "tst_test.h"
+#include "tst_taint.h"
+
+void setup(void)
+{
+	...
+	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
+	...
+}
+...
+void run(void)
+{
+	...
+	if (tst_taint_check() == 0)
+		tst_res(TPASS, "kernel is not tainted");
+	else
+		tst_res(TFAIL, "kernel is tainted");
+}
+-------------------------------------------------------------------------------
+
+You have to call tst_taint_init() with non-zero flags first, preferably during
+setup(). The function will generate a TCONF if the requested flags are not
+fully supported on the running kernel, and TBROK if either a zero mask was
+supplied or if the kernel is already tainted before executing the test.
+
+Then you can call tst_taint_check() during run(), which returns 0 or the 
+tainted flags set in /proc/sys/kernel/tainted as specified earlier.
+
+Depending on your kernel version, not all tainted-flags will be supported.
+
+For reference to tainted kernels, see kernel documentation:
+Documentation/admin-guide/tainted-kernels.rst or
+https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
+
 2.3 Writing a testcase in shell
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/include/tst_taint.h b/include/tst_taint.h
new file mode 100644
index 000000000..1039e2ddc
--- /dev/null
+++ b/include/tst_taint.h
@@ -0,0 +1,104 @@
+/*
+ * Copyright (c) 2018 Michael Moese <mmoese@suse.de>
+ *
+ * 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/>.
+ */
+
+/* Usage example
+ *
+ * ...
+ * #include "tst_test.h"
+ * #include "tst_taint.h"
+ * ..
+ * void setup(void)
+ * {
+ *	...
+ *	tst_taint_init(TST_TAINT_W | TST_TAINT_D));
+ *	...
+ * }
+ *
+ * void run(void)
+ * {
+ *	...
+ *	. test code here
+ *	...
+ *	if (tst_taint_check() != 0)
+ *		tst_res(TFAIL, "kernel has issues");
+ *	else
+ *		tst_res(TPASS, "kernel seems to be fine");
+ * }
+ *
+ *
+ *
+ * The above code checks, if the kernel issued a warning (TST_TAINT_W)
+ * or even died (TST_TAINT_D) during test execution.
+ * If these are set after running a test case, we most likely
+ * triggered a kernel bug.
+ */
+
+#ifndef TST_TAINTED_H__
+#define TST_TAINTED_H__
+
+/*
+ * This are all 17 flags that are present in kernel 4.15
+ * see kernel/panic.c in kernel sources
+ *
+ * Not all of them are valid in all kernel versions.
+ */
+#define TST_TAINT_G     (1 <<  0) /* a module with non-GPL license loaded */
+#define TST_TAINT_F     (1 <<  1) /* a module was force-loaded */
+#define TST_TAINT_S     (1 <<  2) /* SMP with Non-SMP kernel */
+#define TST_TAINT_R     (1 <<  3) /* module force unloaded */
+#define TST_TAINT_M     (1 <<  4) /* machine check error occurred */
+#define TST_TAINT_B     (1 <<  5) /* page-release function found bad page */
+#define TST_TAINT_U     (1 <<  6) /* user requested taint flag */
+#define TST_TAINT_D     (1 <<  7) /* kernel died recently - OOPS or BUG */
+#define TST_TAINT_A     (1 <<  8) /* ACPI table has been overwritten */
+#define TST_TAINT_W     (1 <<  9) /* a warning has been issued by kernel */
+#define TST_TAINT_C     (1 << 10) /* driver from drivers/staging was loaded */
+#define TST_TAINT_I     (1 << 11) /* working around BIOS/Firmware bug */
+#define TST_TAINT_O     (1 << 12) /* out of tree module loaded */
+#define TST_TAINT_E     (1 << 13) /* unsigned module was loaded */
+#define TST_TAINT_L     (1 << 14) /* A soft lock-up has previously occurred */
+#define TST_TAINT_K     (1 << 15) /* kernel has been live-patched */
+#define TST_TAINT_X	(1 << 16) /* auxiliary taint, for distro's use */
+
+/*
+ * Initialize and prepare support for checking tainted kernel.
+ *
+ * supply the mask of TAINT-flags you want to check, for example
+ * (TST_TAINT_W | TST_TAINT_D) when you want to check if the kernel issued
+ * a warning or even reported it died.
+ *
+ * This function tests if the requested flags are supported on the
+ * locally running kernel. In case the tainted-flags are already set by
+ * the kernel, there is no reason to continue and TCONF is generated.
+ *
+ * The mask must not be zero.
+ */
+void tst_taint_init(unsigned int mask);
+
+
+/*
+ * check if the tainted flags handed to tst_taint_init() are still not set
+ * during or after running the test.
+ * Calling this function is only allowed after tst_taint_init() was called,
+ * otherwise TBROK will be generated.
+ *
+ * returns 0 or a bitmask of the flags that currently tainted the kernel.
+ */
+unsigned int tst_taint_check(void);
+
+
+#endif /* TST_TAINTED_H__ */
diff --git a/lib/tst_taint.c b/lib/tst_taint.c
new file mode 100644
index 000000000..8d7a37b47
--- /dev/null
+++ b/lib/tst_taint.c
@@ -0,0 +1,106 @@
+#define TST_NO_DEFAULT_MAIN
+
+#include "tst_test.h"
+#include "tst_taint.h"
+#include "tst_safe_stdio.h"
+
+#define TAINT_FILE "/proc/sys/kernel/tainted"
+
+static unsigned int taint_mask = -1;
+
+static unsigned int tst_taint_read(void)
+{
+	unsigned int val;
+
+	if (taint_mask == (unsigned int) -1)
+		tst_brk(TBROK, "need to call tst_taint_init() first");
+
+	SAFE_FILE_SCANF(TAINT_FILE, "%u", &val);
+
+	return val;
+}
+
+static int tst_taint_check_kver(unsigned int mask)
+{
+	int r1;
+	int r2;
+	int r3 = 0;
+
+	if (mask & TST_TAINT_X) {
+		r1 = 4;
+		r2 = 15;
+	} else if (mask & TST_TAINT_K) {
+		r1 = 4;
+		r2 = 0;
+	} else if (mask & TST_TAINT_L) {
+		r1 = 3;
+		r2 = 17;
+	} else if (mask & TST_TAINT_E) {
+		r1 = 3;
+		r2 = 15;
+	} else if (mask & TST_TAINT_O) {
+		r1 = 3;
+		r2 = 2;
+	} else if (mask & TST_TAINT_I) {
+		r1 = 2;
+		r2 = 6;
+		r3 = 35;
+	} else if (mask & TST_TAINT_C) {
+		r1 = 2;
+		r2 = 6;
+		r3 = 28;
+	} else if (mask & TST_TAINT_W) {
+		r1 = 2;
+		r2 = 6;
+		r3 = 26;
+	} else if (mask & TST_TAINT_A) {
+		r1 = 2;
+		r2 = 6;
+		r3 = 25;
+	} else if (mask & TST_TAINT_D) {
+		r1 = 2;
+		r2 = 6;
+		r3 = 23;
+	} else if (mask & TST_TAINT_U) {
+		r1 = 2;
+		r2 = 6;
+		r3 = 21;
+	} else {
+		r1 = 2;
+		r2 = 6;
+		r3 = 16;
+	}
+
+	return tst_kvercmp(r1, r2, r3);
+}
+
+void tst_taint_init(unsigned int mask)
+{
+	unsigned int taint = -1;
+
+	if (mask == 0)
+		tst_brk(TBROK, "mask is not allowed to be 0");
+
+	if (tst_taint_check_kver(mask) < 0)
+		tst_res(TCONF, "Kernel is too old for requested mask");
+
+	taint_mask = mask;
+
+	taint = tst_taint_read();
+	if ((taint & mask) != 0)
+		tst_brk(TBROK, "Kernel is already tainted: %u", taint);
+}
+
+
+unsigned int tst_taint_check(void)
+{
+	unsigned int taint = -1;
+
+	if (taint_mask == (unsigned int) -1)
+		tst_brk(TBROK, "need to call tst_taint_init() first");
+
+	taint = tst_taint_read();
+
+	return (taint & taint_mask);
+}
+
-- 
2.13.6

[LTP] [PATCH v5 2/3] Add a library wrapper for sigaction()

[Michael Moese]

In a multithreaded program, using signal() results in unspecified
behavior. In this case, sigaction() has to be used to install a
signal handler.
Therefore, SAFE_SIGACTION() is added.

Signed-off-by: Michael Moese <mmoese@suse.de>
---
 include/tst_safe_macros.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 06bff13c7..bf12e0719 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -397,6 +397,26 @@ static inline sighandler_t safe_signal(const char *file, const int lineno,
 #define SAFE_SIGNAL(signum, handler) \
 	safe_signal(__FILE__, __LINE__, (signum), (handler))
 
+
+
+static inline int safe_sigaction(const char *file, const int lineno,
+				 int signum, const struct sigaction *act,
+				 struct sigaction *oldact)
+{
+	int rval;
+
+	rval = sigaction(signum, act, oldact);
+
+	if (rval == -1) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			"sigaction(%d, %p, %p) failed", signum, act, oldact);
+	}
+
+	return rval;
+}
+#define SAFE_SIGACTION(signum, act, oldact) \
+	safe_sigaction(__FILE__, __LINE__, (signum), (act), (oldact))
+
 #define SAFE_EXECLP(file, arg, ...) do {                   \
 	execlp((file), (arg), ##__VA_ARGS__);              \
 	tst_brk_(__FILE__, __LINE__, TBROK | TERRNO,       \
-- 
2.13.6

[LTP] [PATCH v5 3/3] Add regression test for CVE-2017-17053

[Michael Moese]

This patch adds a regression test for CVE-2017-17053, based on the
reproducer in the message of this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccd5b3235180eef3cfec337df1c8554ab151b5cc

Be warned, if the running kernel is vulnerable to this CVE, it will die in
most cases.

Signed-off-by: Michael Moese <mmoese@suse.de>

---
Changes to v4: Reworked after review
---
 runtest/cve                    |   1 +
 testcases/cve/.gitignore       |   1 +
 testcases/cve/Makefile         |   1 +
 testcases/cve/cve-2017-17053.c | 171 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 174 insertions(+)
 create mode 100644 testcases/cve/cve-2017-17053.c

diff --git a/runtest/cve b/runtest/cve
index 6de2ed0ac..6efffc668 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -29,3 +29,4 @@ cve-2017-17807 request_key04
 cve-2017-1000364 stack_clash
 cve-2017-5754 meltdown
 cve-2017-17052 cve-2017-17052
+cve-2017-17053 cve-2017-17053
diff --git a/testcases/cve/.gitignore b/testcases/cve/.gitignore
index 42f32e825..5ecaaeb76 100644
--- a/testcases/cve/.gitignore
+++ b/testcases/cve/.gitignore
@@ -11,3 +11,4 @@ cve-2017-5669
 meltdown
 stack_clash
 cve-2017-17052
+cve-2017-17053
diff --git a/testcases/cve/Makefile b/testcases/cve/Makefile
index 38ce27c93..00fbc1050 100644
--- a/testcases/cve/Makefile
+++ b/testcases/cve/Makefile
@@ -37,5 +37,6 @@ meltdown: CFLAGS += -msse2
 endif
 
 cve-2017-17052:	CFLAGS += -pthread
+cve-2017-17053:	CFLAGS += -pthread
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/cve/cve-2017-17053.c b/testcases/cve/cve-2017-17053.c
new file mode 100644
index 000000000..4f9718e19
--- /dev/null
+++ b/testcases/cve/cve-2017-17053.c
@@ -0,0 +1,171 @@
+/*
+ * Copyright (c) 2018 Michael Moese <mmoese@suse.com>
+ *
+ * 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 CVE-2017-17053, original reproducer can be found
+ * here:
+ * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccd5b3235180eef3cfec337df1c8554ab151b5cc
+ *
+ * Be careful! This test may crash your kernel!
+ */
+
+#include <asm/ldt.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <sys/syscall.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "tst_taint.h"
+#include "lapi/syscalls.h"
+
+#define EXEC_USEC   5000000
+
+static int try_pthread_create(pthread_t *thread_id, const pthread_attr_t *attr,
+			      void *(*thread_fn)(void *), void *arg)
+{
+	int rval;
+
+	errno = 0;
+	rval = pthread_create(thread_id, attr, thread_fn, arg);
+
+	if (rval && errno != EAGAIN && errno != EWOULDBLOCK) {
+		tst_brk(TBROK, "pthread_create(%p,%p,%p,%p) failed: %s",
+			thread_id, attr, thread_fn, arg, tst_strerrno(rval));
+		return rval;
+	}
+	return 0;
+}
+
+static int try_fork(void)
+{
+	pid_t pid;
+
+	fflush(stdout);
+
+	pid = fork();
+	errno = 0;
+	if (pid < 0 && errno != EAGAIN && errno == EWOULDBLOCK)
+		tst_brk(TBROK | TERRNO, "fork() failed");
+	return pid;
+}
+
+
+
+struct shm_data {
+	sig_atomic_t do_exit;
+	sig_atomic_t segfaulted;
+};
+static struct shm_data *shm;
+
+static void handler(int sig)
+{
+	(void)(sig);
+
+	shm->segfaulted = 1;
+}
+
+static void install_sighandler(void)
+{
+	struct sigaction sa;
+
+	sa.sa_flags = SA_SIGINFO;
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = handler;
+
+	SAFE_SIGACTION(SIGSEGV, &sa, NULL);
+}
+
+static void setup(void)
+{
+	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
+
+	shm = SAFE_MMAP(NULL, sizeof(struct shm_data),
+			PROT_READ | PROT_WRITE,
+			MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+}
+
+static void cleanup(void)
+{
+	SAFE_MUNMAP(shm, sizeof(struct shm_data));
+}
+
+static void *fork_thread(void *arg)
+{
+	try_fork();
+	return arg;
+}
+
+void run_test(void)
+{
+	struct user_desc desc = { .entry_number = 8191 };
+
+	install_sighandler();
+	syscall(__NR_modify_ldt, 1, &desc, sizeof(desc));
+
+	for (;;) {
+		if (shm->do_exit)
+			exit(0);
+		else if (shm->segfaulted)
+			exit(-1);
+
+		if (try_fork() == 0) {
+			pthread_t t;
+
+			srand(getpid());
+			errno = 0;
+			if (try_pthread_create(&t, NULL, fork_thread, NULL) != 0)
+				tst_brk(TCONF, "cannot create threads");
+			usleep(rand() % 10000);
+			syscall(__NR_exit_group, 0);
+		}
+	}
+}
+
+void run(void)
+{
+	int status;
+	pid_t pid;
+
+	shm->do_exit = 0;
+	shm->segfaulted = 0;
+	pid = try_fork();
+
+	if (pid == 0) {
+		run_test();
+	} else if (pid > 0) {
+		usleep(EXEC_USEC);
+		shm->do_exit = 1;
+	} else {
+		tst_brk(TCONF, "cannot fork new process");
+	}
+
+	SAFE_WAIT(&status);
+
+	if (WIFEXITED(status) && (shm->segfaulted == 0) && !tst_taint_check())
+		tst_res(TPASS, "kernel survived");
+	else
+		tst_res(TFAIL, "kernel is vulnerable");
+}
+
+static struct tst_test test = {
+	.forks_child = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run,
+};
-- 
2.13.6

[LTP] [PATCH v5 3/3] Add regression test for CVE-2017-17053

[Cyril Hrubis]

Hi!
> +static int try_pthread_create(pthread_t *thread_id, const pthread_attr_t *attr,
> +			      void *(*thread_fn)(void *), void *arg)
> +{
> +	int rval;
> +
> +	errno = 0;
> +	rval = pthread_create(thread_id, attr, thread_fn, arg);
> +
> +	if (rval && errno != EAGAIN && errno != EWOULDBLOCK) {

Unlike the rest of the UNIX API the pthread_* functions returns errno
directly, hence this should be:

if (rval && rval != EAGAIN && rval != EWOULDBLOCK)

> +		tst_brk(TBROK, "pthread_create(%p,%p,%p,%p) failed: %s",
> +			thread_id, attr, thread_fn, arg, tst_strerrno(rval));
> +		return rval;

We will never reach this return, remember the tst_brk() exits the test.

> +	}
> +	return 0;
> +}
> +
> +static int try_fork(void)
> +{
> +	pid_t pid;
> +
> +	fflush(stdout);

We should really do something about this as well. First of all the
test library apparently flushes wrong stream apparently, but that is my
mistake.

Secondly we should not hardcode which streem to flush here in the test.

I guess that best solution would be adding tst_flush(void) to the test
library because the read_all test from ritchie needs that one as well. I
can push that change myself or you can send a patch, your choice :-).

> +	pid = fork();
> +	errno = 0;

There is no reason to zero the errno here.

> +	if (pid < 0 && errno != EAGAIN && errno == EWOULDBLOCK)
> +		tst_brk(TBROK | TERRNO, "fork() failed");
> +	return pid;
> +}
> +
> +
> +
> +struct shm_data {
> +	sig_atomic_t do_exit;
> +	sig_atomic_t segfaulted;
> +};
> +static struct shm_data *shm;

We are changin it from signal handler so it should be volatile as well.

> +static void handler(int sig)
> +{
> +	(void)(sig);

No need for the braces around sig here.

> +	shm->segfaulted = 1;
> +}
> +
> +static void install_sighandler(void)
> +{
> +	struct sigaction sa;
> +
> +	sa.sa_flags = SA_SIGINFO;
> +	sigemptyset(&sa.sa_mask);
> +	sa.sa_handler = handler;
> +
> +	SAFE_SIGACTION(SIGSEGV, &sa, NULL);
> +}
> +
> +static void setup(void)
> +{
> +	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
> +
> +	shm = SAFE_MMAP(NULL, sizeof(struct shm_data),
> +			PROT_READ | PROT_WRITE,
> +			MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_MUNMAP(shm, sizeof(struct shm_data));
> +}
> +
> +static void *fork_thread(void *arg)
> +{
> +	try_fork();
> +	return arg;
> +}
> +
> +void run_test(void)
> +{
> +	struct user_desc desc = { .entry_number = 8191 };
> +
> +	install_sighandler();
> +	syscall(__NR_modify_ldt, 1, &desc, sizeof(desc));
> +
> +	for (;;) {
> +		if (shm->do_exit)
> +			exit(0);
> +		else if (shm->segfaulted)
> +			exit(-1);

Is there a reason to pass a different value to exit() here?

We does not seem to use it in the run() function anyways.

Also passing -1 to exit() is questionable choice since the actual value
reported to the parent is masked with 0xff hence we will end up with 255
in the parent (because of two complement representation of -1).

> +		if (try_fork() == 0) {
> +			pthread_t t;
> +
> +			srand(getpid());
> +			errno = 0;

There is no need to zero the errno here.

> +			if (try_pthread_create(&t, NULL, fork_thread, NULL) != 0)
> +				tst_brk(TCONF, "cannot create threads");

See above, we will never call the tst_brk() here.

> +			usleep(rand() % 10000);
> +			syscall(__NR_exit_group, 0);
> +		}
> +	}
> +}
> +
> +void run(void)
> +{
> +	int status;
> +	pid_t pid;
> +
> +	shm->do_exit = 0;
> +	shm->segfaulted = 0;
> +	pid = try_fork();
> +
> +	if (pid == 0) {
> +		run_test();
> +	} else if (pid > 0) {
> +		usleep(EXEC_USEC);
> +		shm->do_exit = 1;
> +	} else {
> +		tst_brk(TCONF, "cannot fork new process");
> +	}

Hmm, isn't it safe to call SAFE_FORK() here? The system shouldn't be
memory starved at the point we start the test or am I mistaken?

> +	SAFE_WAIT(&status);
> +
> +	if (WIFEXITED(status) && (shm->segfaulted == 0) && !tst_taint_check())
> +		tst_res(TPASS, "kernel survived");
> +	else
> +		tst_res(TFAIL, "kernel is vulnerable");
> +}
> +
> +static struct tst_test test = {
> +	.forks_child = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +};
> -- 
> 2.13.6
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v5 3/3] Add regression test for CVE-2017-17053

[Michael Moese]

Hi,

On Wed, Mar 07, 2018 at 03:14:45PM +0100, Cyril Hrubis wrote:
> Hi!
> > +static int try_pthread_create(pthread_t *thread_id, const pthread_attr_t *attr,
> > +			      void *(*thread_fn)(void *), void *arg)
> > +{
> > +	int rval;
> > +
> > +	errno = 0;
> > +	rval = pthread_create(thread_id, attr, thread_fn, arg);
> > +
> > +	if (rval && errno != EAGAIN && errno != EWOULDBLOCK) {
> 
> Unlike the rest of the UNIX API the pthread_* functions returns errno
> directly, hence this should be:
> 
> if (rval && rval != EAGAIN && rval != EWOULDBLOCK)
> 
Ok, that sounds reasonable. I'll change that. 

> > +		tst_brk(TBROK, "pthread_create(%p,%p,%p,%p) failed: %s",
> > +			thread_id, attr, thread_fn, arg, tst_strerrno(rval));
> > +		return rval;
> 
> We will never reach this return, remember the tst_brk() exits the test.
>
Ok, removing the return.

> > +	}
> > +	return 0;
> > +}
> > +
> > +static int try_fork(void)
> > +{
> > +	pid_t pid;
> > +
> > +	fflush(stdout);
> 
> We should really do something about this as well. First of all the
> test library apparently flushes wrong stream apparently, but that is my
> mistake.
> 
> Secondly we should not hardcode which streem to flush here in the test.
> 
> I guess that best solution would be adding tst_flush(void) to the test
> library because the read_all test from ritchie needs that one as well. I
> can push that change myself or you can send a patch, your choice :-).
> 
I can send a patch. Should this only flush stdout, or also stderr?

> > +	pid = fork();
> > +	errno = 0;
> 
> There is no reason to zero the errno here.
> 
Well, this is obviously not only without reason but plain wrong. It
has to be the other way round. But I think it must be zeroed before 
fork().

> > +	if (pid < 0 && errno != EAGAIN && errno == EWOULDBLOCK)
> > +		tst_brk(TBROK | TERRNO, "fork() failed");
> > +	return pid;
> > +}
> > +
> > +
> > +
> > +struct shm_data {
> > +	sig_atomic_t do_exit;
> > +	sig_atomic_t segfaulted;
> > +};
> > +static struct shm_data *shm;
> 
> We are changin it from signal handler so it should be volatile as well.
> 
> > +static void handler(int sig)
> > +{
> > +	(void)(sig);
> 
> No need for the braces around sig here.
> 
> > +	shm->segfaulted = 1;
> > +}
> > +
> > +static void install_sighandler(void)
> > +{
> > +	struct sigaction sa;
> > +
> > +	sa.sa_flags = SA_SIGINFO;
> > +	sigemptyset(&sa.sa_mask);
> > +	sa.sa_handler = handler;
> > +
> > +	SAFE_SIGACTION(SIGSEGV, &sa, NULL);
> > +}
> > +
> > +static void setup(void)
> > +{
> > +	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
> > +
> > +	shm = SAFE_MMAP(NULL, sizeof(struct shm_data),
> > +			PROT_READ | PROT_WRITE,
> > +			MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +	SAFE_MUNMAP(shm, sizeof(struct shm_data));
> > +}
> > +
> > +static void *fork_thread(void *arg)
> > +{
> > +	try_fork();
> > +	return arg;
> > +}
> > +
> > +void run_test(void)
> > +{
> > +	struct user_desc desc = { .entry_number = 8191 };
> > +
> > +	install_sighandler();
> > +	syscall(__NR_modify_ldt, 1, &desc, sizeof(desc));
> > +
> > +	for (;;) {
> > +		if (shm->do_exit)
> > +			exit(0);
> > +		else if (shm->segfaulted)
> > +			exit(-1);
> 
> Is there a reason to pass a different value to exit() here?
> 
> We does not seem to use it in the run() function anyways.
> 
I think we should use it. I'll add WEXITSTATUS(status) the check 
if the test passed. 

> Also passing -1 to exit() is questionable choice since the actual value
> reported to the parent is masked with 0xff hence we will end up with 255
> in the parent (because of two complement representation of -1).
>
I'll change 0 and -1 to EXIT_SUCCESS and EXIT_FAILURE. In the end, we 
don't care for more.
 
> > +		if (try_fork() == 0) {
> > +			pthread_t t;
> > +
> > +			srand(getpid());
> > +			errno = 0;
> 
> There is no need to zero the errno here.
> 
True.
> > +			if (try_pthread_create(&t, NULL, fork_thread, NULL) != 0)
> > +				tst_brk(TCONF, "cannot create threads");
> 
> See above, we will never call the tst_brk() here.
> 
> > +			usleep(rand() % 10000);
> > +			syscall(__NR_exit_group, 0);
> > +		}
> > +	}
> > +}
> > +
> > +void run(void)
> > +{
> > +	int status;
> > +	pid_t pid;
> > +
> > +	shm->do_exit = 0;
> > +	shm->segfaulted = 0;
> > +	pid = try_fork();
> > +
> > +	if (pid == 0) {
> > +		run_test();
> > +	} else if (pid > 0) {
> > +		usleep(EXEC_USEC);
> > +		shm->do_exit = 1;
> > +	} else {
> > +		tst_brk(TCONF, "cannot fork new process");
> > +	}
> 
> Hmm, isn't it safe to call SAFE_FORK() here? The system shouldn't be
> memory starved at the point we start the test or am I mistaken?
> 
It does not really make a difference. Used it just to have it more homogeneous
throughout the file. Changing it to SAFE_FORK() should not matter.

> > +	SAFE_WAIT(&status);
> > +
> > +	if (WIFEXITED(status) && (shm->segfaulted == 0) && !tst_taint_check())
> > +		tst_res(TPASS, "kernel survived");
> > +	else
> > +		tst_res(TFAIL, "kernel is vulnerable");
> > +}
> > +
> > +static struct tst_test test = {
> > +	.forks_child = 1,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.test_all = run,
> > +};
> > -- 
> > 2.13.6
> > 
> > 
> > -- 
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

Michael
-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

[LTP] [PATCH v5 3/3] Add regression test for CVE-2017-17053

[Cyril Hrubis]

Hi!
> > > +static int try_pthread_create(pthread_t *thread_id, const pthread_attr_t *attr,
> > > +			      void *(*thread_fn)(void *), void *arg)
> > > +{
> > > +	int rval;
> > > +
> > > +	errno = 0;
> > > +	rval = pthread_create(thread_id, attr, thread_fn, arg);
> > > +
> > > +	if (rval && errno != EAGAIN && errno != EWOULDBLOCK) {
> > 
> > Unlike the rest of the UNIX API the pthread_* functions returns errno
> > directly, hence this should be:
> > 
> > if (rval && rval != EAGAIN && rval != EWOULDBLOCK)
> > 
> Ok, that sounds reasonable. I'll change that. 

FYI using errno with phread_* functions is plain wrong as this code would
behave randomly since the value of errno here would be set by the last
failing call.

> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static int try_fork(void)
> > > +{
> > > +	pid_t pid;
> > > +
> > > +	fflush(stdout);
> > 
> > We should really do something about this as well. First of all the
> > test library apparently flushes wrong stream apparently, but that is my
> > mistake.
> > 
> > Secondly we should not hardcode which streem to flush here in the test.
> > 
> > I guess that best solution would be adding tst_flush(void) to the test
> > library because the read_all test from ritchie needs that one as well. I
> > can push that change myself or you can send a patch, your choice :-).
> > 
> I can send a patch. Should this only flush stdout, or also stderr?

The test library writes everything into stderr, see fputs() at the end
of the print_result(). Hence we should at least flush stderr. Flushing
stdout as well would not harm, it should be empty unless the test
wrote anything there.

> > > +	pid = fork();
> > > +	errno = 0;
> > 
> > There is no reason to zero the errno here.
> > 
> Well, this is obviously not only without reason but plain wrong. It
> has to be the other way round. But I think it must be zeroed before 
> fork().

The errno is set on failure, so if we got -1 from fork() it has been set
in glibc to something meaningful. I do not see a reason to reset it
before fork().

There are a few calls that do not have a well defined return value that
represents a failure and because of that  errno has to be reset before
we call them (for example the strto* functions) but these quire rare.

> > > +	struct user_desc desc = { .entry_number = 8191 };
> > > +
> > > +	install_sighandler();
> > > +	syscall(__NR_modify_ldt, 1, &desc, sizeof(desc));
> > > +
> > > +	for (;;) {
> > > +		if (shm->do_exit)
> > > +			exit(0);
> > > +		else if (shm->segfaulted)
> > > +			exit(-1);
> > 
> > Is there a reason to pass a different value to exit() here?
> > 
> > We does not seem to use it in the run() function anyways.
> > 
> I think we should use it. I'll add WEXITSTATUS(status) the check 
> if the test passed. 

We do check the shm->segfaulted there so there is no need to propagate
it in the exit value as well...

> > > +void run(void)
> > > +{
> > > +	int status;
> > > +	pid_t pid;
> > > +
> > > +	shm->do_exit = 0;
> > > +	shm->segfaulted = 0;
> > > +	pid = try_fork();
> > > +
> > > +	if (pid == 0) {
> > > +		run_test();
> > > +	} else if (pid > 0) {
> > > +		usleep(EXEC_USEC);
> > > +		shm->do_exit = 1;
> > > +	} else {
> > > +		tst_brk(TCONF, "cannot fork new process");
> > > +	}
> > 
> > Hmm, isn't it safe to call SAFE_FORK() here? The system shouldn't be
> > memory starved at the point we start the test or am I mistaken?
> > 
> It does not really make a difference. Used it just to have it more homogeneous
> throughout the file. Changing it to SAFE_FORK() should not matter.

Well the test-writing-guidelines says that we should use SAFE_MACROS()
whenever possible. In this case it adds a bit of confusion since the
reader of the code would wonder why SAFE_FORK() is not used.

Also the tst_brk() message doesn't have the TERRNO flag, so if the fork
has failed here you would have no idea why when looking at the logs.

So please let's keep the error messages consistent and use raw fork()
only when we have to.

> > > +	SAFE_WAIT(&status);
> > > +
> > > +	if (WIFEXITED(status) && (shm->segfaulted == 0) && !tst_taint_check())
> > > +		tst_res(TPASS, "kernel survived");
> > > +	else
> > > +		tst_res(TFAIL, "kernel is vulnerable");
> > > +}
> > > +
> > > +static struct tst_test test = {
> > > +	.forks_child = 1,
> > > +	.setup = setup,
> > > +	.cleanup = cleanup,
> > > +	.test_all = run,
> > > +};
> > > -- 
> > > 2.13.6
> > > 
> > > 
> > > -- 
> > > Mailing list info: https://lists.linux.it/listinfo/ltp
> > 
> > -- 
> > Cyril Hrubis
> > chrubis@suse.cz
> 
> Michael
> -- 
> SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton, HRB 21284 (AG N?rnberg)

-- 
Cyril Hrubis
chrubis@suse.cz