[PATCH v3 0/4] sysctl: fix incorrect write position handling

[PATCH v3 0/4] sysctl: fix incorrect write position handling

[Kees Cook]

When writing to a sysctl string, each write, regardless of VFS position,
began writing the string from the start. This meant the contents of
the last write to the sysctl controlled the string contents instead of
the first.

This misbehavior was featured in an exploit against Chrome OS. While it's
not in itself a vulnerability, it's a weirdness that isn't on the mind
of most auditors: "This filter looks correct, the first line written
would not be meaningful to sysctl" doesn't apply here, since the size
of the write and the contents of the final write are what matter when
writing to sysctls.

This adds the sysctl kernel.sysctl_writes_strict to control the write
behavior. The default (0) reports when VFS position is non-0 on a write,
but retains legacy behavior, -1 disables the warning, and 1 enables the
position-respecting behavior.

Thanks,

-Kees

[PATCH 1/4] sysctl: clean up char buffer arguments

[Kees Cook]

The char buffer arguments are needlessly cast in weird places. Clean it up
so things are easier to read.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/sysctl.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 74f5b580fe34..e7ff80a73c44 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1698,8 +1698,8 @@ int __init sysctl_init(void)
 
 #ifdef CONFIG_PROC_SYSCTL
 
-static int _proc_do_string(void* data, int maxlen, int write,
-			   void __user *buffer,
+static int _proc_do_string(char *data, int maxlen, int write,
+			   char __user *buffer,
 			   size_t *lenp, loff_t *ppos)
 {
 	size_t len;
@@ -1725,7 +1725,7 @@ static int _proc_do_string(void* data, int maxlen, int write,
 			len = maxlen-1;
 		if(copy_from_user(data, buffer, len))
 			return -EFAULT;
-		((char *) data)[len] = 0;
+		data[len] = 0;
 		*ppos += *lenp;
 	} else {
 		len = strlen(data);
@@ -1743,10 +1743,10 @@ static int _proc_do_string(void* data, int maxlen, int write,
 		if (len > *lenp)
 			len = *lenp;
 		if (len)
-			if(copy_to_user(buffer, data, len))
+			if (copy_to_user(buffer, data, len))
 				return -EFAULT;
 		if (len < *lenp) {
-			if(put_user('\n', ((char __user *) buffer) + len))
+			if (put_user('\n', buffer + len))
 				return -EFAULT;
 			len++;
 		}
@@ -1776,8 +1776,8 @@ static int _proc_do_string(void* data, int maxlen, int write,
 int proc_dostring(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	return _proc_do_string(table->data, table->maxlen, write,
-			       buffer, lenp, ppos);
+	return _proc_do_string((char *)(table->data), table->maxlen, write,
+			       (char __user *)buffer, lenp, ppos);
 }
 
 static size_t proc_skip_spaces(char **buf)
-- 
1.7.9.5

[PATCH 2/4] sysctl: refactor sysctl string writing logic

[Kees Cook]

Consolidate buffer length checking with new-line/end-of-line checking.
Additionally, instead of reading user memory twice, just do the assignment
during the loop.

This change doesn't affect the potential races here. It was already
possible to read a sysctl that was in the middle of a write. In both
cases, the string will always be NULL terminated. The pre-existing race
remains a problem to be solved.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/sysctl.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e7ff80a73c44..0e08103a69c8 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1712,21 +1712,18 @@ static int _proc_do_string(char *data, int maxlen, int write,
 	}
 
 	if (write) {
+		/* Start writing from beginning of buffer. */
 		len = 0;
+		*ppos += *lenp;
 		p = buffer;
-		while (len < *lenp) {
+		while ((p - buffer) < *lenp && len < maxlen - 1) {
 			if (get_user(c, p++))
 				return -EFAULT;
 			if (c == 0 || c == '\n')
 				break;
-			len++;
+			data[len++] = c;
 		}
-		if (len >= maxlen)
-			len = maxlen-1;
-		if(copy_from_user(data, buffer, len))
-			return -EFAULT;
 		data[len] = 0;
-		*ppos += *lenp;
 	} else {
 		len = strlen(data);
 		if (len > maxlen)
-- 
1.7.9.5

[PATCH 3/4] sysctl: allow for strict write position handling

[Kees Cook]

When writing to a sysctl string, each write, regardless of VFS position,
begins writing the string from the start. This means the contents of
the last write to the sysctl controls the string contents instead of
the first:

open("/proc/sys/kernel/modprobe", O_WRONLY)   = 1
write(1, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"..., 4096) = 4096
write(1, "/bin/true", 9)                = 9
close(1)                                = 0

$ cat /proc/sys/kernel/modprobe
/bin/true

Expected behaviour would be to have the sysctl be "AAAA..." capped at
maxlen (in this case KMOD_PATH_LEN: 256), instead of truncating to the
contents of the second write. Similarly, multiple short writes would
not append to the sysctl.

The old behavior is unlike regular POSIX files enough that doing audits of
software that interact with sysctls can end up in unexpected or dangerous
situations. For example, "as long as the input starts with a trusted
path" turns out to be an insufficient filter, as what must also happen
is for the input to be entirely contained in a single write syscall --
not a common consideration, especially for high level tools.

This provides kernel.sysctl_writes_strict as a way to make this behavior
act in a less surprising manner for strings, and disallows non-zero
file position when writing numeric sysctls (similar to what is already
done when reading from non-zero file positions). For now, the default
(0) is to warn about non-zero file position use, but retain the legacy
behavior. Setting this to -1 disables the warning, and setting this to
1 enables the file position respecting behavior.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/sysctl/kernel.txt |   21 ++++++++++++
 kernel/sysctl.c                 |   69 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9886c3d57fc2..708bb7f1b7e0 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -77,6 +77,7 @@ show up in /proc/sys/kernel:
 - shmmni
 - stop-a                      [ SPARC only ]
 - sysrq                       ==> Documentation/sysrq.txt
+- sysctl_writes_strict
 - tainted
 - threads-max
 - unknown_nmi_panic
@@ -762,6 +763,26 @@ without users and with a dead originative process will be destroyed.
 
 ==============================================================
 
+sysctl_writes_strict:
+
+Control how file position affects the behavior of updating sysctl values
+via the /proc/sys interface:
+
+  -1 - Legacy per-write sysctl value handling, with no printk warnings.
+       Each write syscall must fully contain the sysctl value to be
+       written, and multiple writes on the same sysctl file descriptor
+       will rewrite the sysctl value, regardless of file position.
+   0 - (default) Same behavior as above, but warn about processes that
+       perform writes to a sysctl file descriptor when the file position
+       is not 0.
+   1 - Respect file position when writing sysctl strings. Multiple writes
+       will append to the sysctl value buffer. Anything past the max length
+       of the sysctl value buffer will be ignored. Writes to numeric sysctl
+       entries must always be at file position 0 and the value must be
+       fully contained in the buffer sent in the write syscall.
+
+==============================================================
+
 tainted:
 
 Non-zero if the kernel has been tainted.  Numeric values, which
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0e08103a69c8..d5bee7949e4a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -173,6 +173,13 @@ extern int no_unaligned_warning;
 #endif
 
 #ifdef CONFIG_PROC_SYSCTL
+
+#define SYSCTL_WRITES_LEGACY	-1
+#define SYSCTL_WRITES_WARN	 0
+#define SYSCTL_WRITES_STRICT	 1
+
+static int sysctl_writes_strict = SYSCTL_WRITES_WARN;
+
 static int proc_do_cad_pid(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos);
 static int proc_taint(struct ctl_table *table, int write,
@@ -495,6 +502,15 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_taint,
 	},
+	{
+		.procname	= "sysctl_writes_strict",
+		.data		= &sysctl_writes_strict,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &neg_one,
+		.extra2		= &one,
+	},
 #endif
 #ifdef CONFIG_LATENCYTOP
 	{
@@ -1712,8 +1728,20 @@ static int _proc_do_string(char *data, int maxlen, int write,
 	}
 
 	if (write) {
-		/* Start writing from beginning of buffer. */
-		len = 0;
+		if (sysctl_writes_strict == SYSCTL_WRITES_STRICT) {
+			/* Only continue writes not past the end of buffer. */
+			len = strlen(data);
+			if (len > maxlen - 1)
+				len = maxlen - 1;
+
+			if (*ppos > len)
+				return 0;
+			len = *ppos;
+		} else {
+			/* Start writing from beginning of buffer. */
+			len = 0;
+		}
+
 		*ppos += *lenp;
 		p = buffer;
 		while ((p - buffer) < *lenp && len < maxlen - 1) {
@@ -1753,6 +1781,14 @@ static int _proc_do_string(char *data, int maxlen, int write,
 	return 0;
 }
 
+static void warn_sysctl_write(struct ctl_table *table)
+{
+	pr_warn("%s wrote to %s when file position was not 0!\n",
+		current->task_comm, table->procname);
+	pr_warn("This will not be supported in the future.\n");
+	pr_warn("To silence warning, set kernel.sysctl_writes_strict = -1\n");
+}
+
 /**
  * proc_dostring - read a string sysctl
  * @table: the sysctl table
@@ -1773,6 +1809,9 @@ static int _proc_do_string(char *data, int maxlen, int write,
 int proc_dostring(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos)
 {
+	if (write && *ppos && sysctl_writes_strict == SYSCTL_WRITES_WARN)
+		warn_sysctl_write(table);
+
 	return _proc_do_string((char *)(table->data), table->maxlen, write,
 			       (char __user *)buffer, lenp, ppos);
 }
@@ -1948,6 +1987,18 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 		conv = do_proc_dointvec_conv;
 
 	if (write) {
+		if (*ppos) {
+			switch (sysctl_writes_strict) {
+			case SYSCTL_WRITES_STRICT:
+				goto out;
+			case SYSCTL_WRITES_WARN:
+				warn_sysctl_write(table);
+				break;
+			default:
+				break;
+			}
+		}
+
 		if (left > PAGE_SIZE - 1)
 			left = PAGE_SIZE - 1;
 		page = __get_free_page(GFP_TEMPORARY);
@@ -2005,6 +2056,7 @@ free:
 			return err ? : -EINVAL;
 	}
 	*lenp -= left;
+out:
 	*ppos += *lenp;
 	return err;
 }
@@ -2197,6 +2249,18 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 	left = *lenp;
 
 	if (write) {
+		if (*ppos) {
+			switch (sysctl_writes_strict) {
+			case SYSCTL_WRITES_STRICT:
+				goto out;
+			case SYSCTL_WRITES_WARN:
+				warn_sysctl_write(table);
+				break;
+			default:
+				break;
+			}
+		}
+
 		if (left > PAGE_SIZE - 1)
 			left = PAGE_SIZE - 1;
 		page = __get_free_page(GFP_TEMPORARY);
@@ -2252,6 +2316,7 @@ free:
 			return err ? : -EINVAL;
 	}
 	*lenp -= left;
+out:
 	*ppos += *lenp;
 	return err;
 }
-- 
1.7.9.5

[PATCH 4/4] test: validate sysctl_writes_strict

[Kees Cook]

This adds several behavioral tests to sysctl string and number writing
to detect unexpected cases that behaved differently when the sysctl
kernel.sysctl_writes_strict != 1.

[ original ]
root@localhost:~# make test_num
== Testing sysctl behavior against /proc/sys/kernel/domainname ==
Writing test file ... ok
Checking sysctl is not set to test value ... ok
Writing sysctl from shell ... ok
Resetting sysctl to original value ... ok
Writing entire sysctl in single write ... ok
Writing middle of sysctl after synchronized seek ... FAIL
Writing beyond end of sysctl ... FAIL
Writing sysctl with multiple long writes ... FAIL
Writing entire sysctl in short writes ... FAIL
Writing middle of sysctl after unsynchronized seek ... ok
Checking sysctl maxlen is at least 65 ... ok
Checking sysctl keeps original string on overflow append ... FAIL
Checking sysctl stays NULL terminated on write ... ok
Checking sysctl stays NULL terminated on overwrite ... ok
make: *** [test_num] Error 1
root@localhost:~# make test_string
== Testing sysctl behavior against /proc/sys/vm/swappiness ==
Writing test file ... ok
Checking sysctl is not set to test value ... ok
Writing sysctl from shell ... ok
Resetting sysctl to original value ... ok
Writing entire sysctl in single write ... ok
Writing middle of sysctl after synchronized seek ... FAIL
Writing beyond end of sysctl ... FAIL
Writing sysctl with multiple long writes ... ok
make: *** [test_string] Error 1

[ with CONFIG_PROC_SYSCTL_STRICT_WRITES ]
root@localhost:~# make run_tests
== Testing sysctl behavior against /proc/sys/kernel/domainname ==
Writing test file ... ok
Checking sysctl is not set to test value ... ok
Writing sysctl from shell ... ok
Resetting sysctl to original value ... ok
Writing entire sysctl in single write ... ok
Writing middle of sysctl after synchronized seek ... ok
Writing beyond end of sysctl ... ok
Writing sysctl with multiple long writes ... ok
Writing entire sysctl in short writes ... ok
Writing middle of sysctl after unsynchronized seek ... ok
Checking sysctl maxlen is at least 65 ... ok
Checking sysctl keeps original string on overflow append ... ok
Checking sysctl stays NULL terminated on write ... ok
Checking sysctl stays NULL terminated on overwrite ... ok
== Testing sysctl behavior against /proc/sys/vm/swappiness ==
Writing test file ... ok
Checking sysctl is not set to test value ... ok
Writing sysctl from shell ... ok
Resetting sysctl to original value ... ok
Writing entire sysctl in single write ... ok
Writing middle of sysctl after synchronized seek ... ok
Writing beyond end of sysctl ... ok
Writing sysctl with multiple long writes ... ok

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/sysctl.c                                 |    4 +-
 tools/testing/selftests/Makefile                |    1 +
 tools/testing/selftests/sysctl/Makefile         |   19 ++++
 tools/testing/selftests/sysctl/common_tests     |  109 +++++++++++++++++++++++
 tools/testing/selftests/sysctl/run_numerictests |   10 +++
 tools/testing/selftests/sysctl/run_stringtests  |   77 ++++++++++++++++
 6 files changed, 218 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/sysctl/Makefile
 create mode 100644 tools/testing/selftests/sysctl/common_tests
 create mode 100644 tools/testing/selftests/sysctl/run_numerictests
 create mode 100644 tools/testing/selftests/sysctl/run_stringtests

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index d5bee7949e4a..51bbf904d694 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1783,8 +1783,8 @@ static int _proc_do_string(char *data, int maxlen, int write,
 
 static void warn_sysctl_write(struct ctl_table *table)
 {
-	pr_warn("%s wrote to %s when file position was not 0!\n",
-		current->task_comm, table->procname);
+	pr_warn("write to %s by %s used a non-zero file position!\n",
+		table->procname, current->comm);
 	pr_warn("This will not be supported in the future.\n");
 	pr_warn("To silence warning, set kernel.sysctl_writes_strict = -1\n");
 }
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 32487ed18354..e66e710cc595 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -10,6 +10,7 @@ TARGETS += timers
 TARGETS += vm
 TARGETS += powerpc
 TARGETS += user
+TARGETS += sysctl
 
 all:
 	for TARGET in $(TARGETS); do \
diff --git a/tools/testing/selftests/sysctl/Makefile b/tools/testing/selftests/sysctl/Makefile
new file mode 100644
index 000000000000..0a92adaf0865
--- /dev/null
+++ b/tools/testing/selftests/sysctl/Makefile
@@ -0,0 +1,19 @@
+# Makefile for sysctl selftests.
+# Expects kernel.sysctl_writes_strict=1.
+
+# No binaries, but make sure arg-less "make" doesn't trigger "run_tests".
+all:
+
+# Allow specific tests to be selected.
+test_num:
+	@/bin/sh ./run_numerictests
+
+test_string:
+	@/bin/sh ./run_stringtests
+
+run_tests: all test_num test_string
+
+# Nothing to clean up.
+clean:
+
+.PHONY: all run_tests clean test_num test_string
diff --git a/tools/testing/selftests/sysctl/common_tests b/tools/testing/selftests/sysctl/common_tests
new file mode 100644
index 000000000000..17d534b1b7b4
--- /dev/null
+++ b/tools/testing/selftests/sysctl/common_tests
@@ -0,0 +1,109 @@
+#!/bin/sh
+
+TEST_FILE=$(mktemp)
+
+echo "== Testing sysctl behavior against ${TARGET} =="
+
+set_orig()
+{
+	echo "${ORIG}" > "${TARGET}"
+}
+
+set_test()
+{
+	echo "${TEST_STR}" > "${TARGET}"
+}
+
+verify()
+{
+	local seen
+	seen=$(cat "$1")
+	if [ "${seen}" != "${TEST_STR}" ]; then
+		return 1
+	fi
+	return 0
+}
+
+trap 'set_orig; rm -f "${TEST_FILE}"' EXIT
+
+rc=0
+
+echo -n "Writing test file ... "
+echo "${TEST_STR}" > "${TEST_FILE}"
+if ! verify "${TEST_FILE}"; then
+	echo "FAIL" >&2
+	exit 1
+else
+	echo "ok"
+fi
+
+echo -n "Checking sysctl is not set to test value ... "
+if verify "${TARGET}"; then
+	echo "FAIL" >&2
+	exit 1
+else
+	echo "ok"
+fi
+
+echo -n "Writing sysctl from shell ... "
+set_test
+if ! verify "${TARGET}"; then
+	echo "FAIL" >&2
+	exit 1
+else
+	echo "ok"
+fi
+
+echo -n "Resetting sysctl to original value ... "
+set_orig
+if verify "${TARGET}"; then
+	echo "FAIL" >&2
+	exit 1
+else
+	echo "ok"
+fi
+
+# Now that we've validated the sanity of "set_test" and "set_orig",
+# we can use those functions to set starting states before running
+# specific behavioral tests.
+
+echo -n "Writing entire sysctl in single write ... "
+set_orig
+dd if="${TEST_FILE}" of="${TARGET}" bs=4096 2>/dev/null
+if ! verify "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+echo -n "Writing middle of sysctl after synchronized seek ... "
+set_test
+dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 skip=1 2>/dev/null
+if ! verify "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+echo -n "Writing beyond end of sysctl ... "
+set_orig
+dd if="${TEST_FILE}" of="${TARGET}" bs=20 seek=2 2>/dev/null
+if verify "${TARGET}"; then
+        echo "FAIL" >&2
+        rc=1
+else
+        echo "ok"
+fi
+
+echo -n "Writing sysctl with multiple long writes ... "
+set_orig
+(perl -e 'print "A" x 50;'; echo "${TEST_STR}") | \
+	dd of="${TARGET}" bs=50 2>/dev/null
+if verify "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
diff --git a/tools/testing/selftests/sysctl/run_numerictests b/tools/testing/selftests/sysctl/run_numerictests
new file mode 100644
index 000000000000..8510f93f2d14
--- /dev/null
+++ b/tools/testing/selftests/sysctl/run_numerictests
@@ -0,0 +1,10 @@
+#!/bin/sh
+
+SYSCTL="/proc/sys"
+TARGET="${SYSCTL}/vm/swappiness"
+ORIG=$(cat "${TARGET}")
+TEST_STR=$(( $ORIG + 1 ))
+
+. ./common_tests
+
+exit $rc
diff --git a/tools/testing/selftests/sysctl/run_stringtests b/tools/testing/selftests/sysctl/run_stringtests
new file mode 100644
index 000000000000..90a9293d520c
--- /dev/null
+++ b/tools/testing/selftests/sysctl/run_stringtests
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+SYSCTL="/proc/sys"
+TARGET="${SYSCTL}/kernel/domainname"
+ORIG=$(cat "${TARGET}")
+TEST_STR="Testing sysctl"
+
+. ./common_tests
+
+# Only string sysctls support seeking/appending.
+MAXLEN=65
+
+echo -n "Writing entire sysctl in short writes ... "
+set_orig
+dd if="${TEST_FILE}" of="${TARGET}" bs=1 2>/dev/null
+if ! verify "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+echo -n "Writing middle of sysctl after unsynchronized seek ... "
+set_test
+dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 2>/dev/null
+if verify "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+echo -n "Checking sysctl maxlen is at least $MAXLEN ... "
+set_orig
+perl -e 'print "A" x ('"${MAXLEN}"'-2), "B";' | \
+	dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
+if ! grep -q B "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+echo -n "Checking sysctl keeps original string on overflow append ... "
+set_orig
+perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
+	dd of="${TARGET}" bs=$(( MAXLEN - 1 )) 2>/dev/null
+if grep -q B "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+echo -n "Checking sysctl stays NULL terminated on write ... "
+set_orig
+perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
+	dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
+if grep -q B "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+echo -n "Checking sysctl stays NULL terminated on overwrite ... "
+set_orig
+perl -e 'print "A" x ('"${MAXLEN}"'-1), "BB";' | \
+	dd of="${TARGET}" bs=$(( $MAXLEN + 1 )) 2>/dev/null
+if grep -q B "${TARGET}"; then
+	echo "FAIL" >&2
+	rc=1
+else
+	echo "ok"
+fi
+
+exit $rc
-- 
1.7.9.5

Re: [PATCH 4/4] test: validate sysctl_writes_strict

[Randy Dunlap]

On 05/01/2014 02:26 PM, Kees Cook wrote:
> This adds several behavioral tests to sysctl string and number writing
> to detect unexpected cases that behaved differently when the sysctl
> kernel.sysctl_writes_strict != 1.
>
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   kernel/sysctl.c                                 |    4 +-
>   tools/testing/selftests/Makefile                |    1 +
>   tools/testing/selftests/sysctl/Makefile         |   19 ++++
>   tools/testing/selftests/sysctl/common_tests     |  109 +++++++++++++++++++++++
>   tools/testing/selftests/sysctl/run_numerictests |   10 +++
>   tools/testing/selftests/sysctl/run_stringtests  |   77 ++++++++++++++++
>   6 files changed, 218 insertions(+), 2 deletions(-)
>   create mode 100644 tools/testing/selftests/sysctl/Makefile
>   create mode 100644 tools/testing/selftests/sysctl/common_tests
>   create mode 100644 tools/testing/selftests/sysctl/run_numerictests
>   create mode 100644 tools/testing/selftests/sysctl/run_stringtests
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index d5bee7949e4a..51bbf904d694 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1783,8 +1783,8 @@ static int _proc_do_string(char *data, int maxlen, int write,
>
>   static void warn_sysctl_write(struct ctl_table *table)
>   {
> -	pr_warn("%s wrote to %s when file position was not 0!\n",
> -		current->task_comm, table->procname);
> +	pr_warn("write to %s by %s used a non-zero file position!\n",
> +		table->procname, current->comm);
>   	pr_warn("This will not be supported in the future.\n");
>   	pr_warn("To silence warning, set kernel.sysctl_writes_strict = -1\n");
>   }

Why is this patch part of the test patch?

-- 
~Randy

Re: [PATCH 4/4] test: validate sysctl_writes_strict

[Kees Cook]

On Sat, May 3, 2014 at 7:42 AM, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 05/01/2014 02:26 PM, Kees Cook wrote:
>>
>> This adds several behavioral tests to sysctl string and number writing
>> to detect unexpected cases that behaved differently when the sysctl
>> kernel.sysctl_writes_strict != 1.
>>
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>   kernel/sysctl.c                                 |    4 +-
>>   tools/testing/selftests/Makefile                |    1 +
>>   tools/testing/selftests/sysctl/Makefile         |   19 ++++
>>   tools/testing/selftests/sysctl/common_tests     |  109
>> +++++++++++++++++++++++
>>   tools/testing/selftests/sysctl/run_numerictests |   10 +++
>>   tools/testing/selftests/sysctl/run_stringtests  |   77 ++++++++++++++++
>>   6 files changed, 218 insertions(+), 2 deletions(-)
>>   create mode 100644 tools/testing/selftests/sysctl/Makefile
>>   create mode 100644 tools/testing/selftests/sysctl/common_tests
>>   create mode 100644 tools/testing/selftests/sysctl/run_numerictests
>>   create mode 100644 tools/testing/selftests/sysctl/run_stringtests
>>
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index d5bee7949e4a..51bbf904d694 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1783,8 +1783,8 @@ static int _proc_do_string(char *data, int maxlen,
>> int write,
>>
>>   static void warn_sysctl_write(struct ctl_table *table)
>>   {
>> -       pr_warn("%s wrote to %s when file position was not 0!\n",
>> -               current->task_comm, table->procname);
>> +       pr_warn("write to %s by %s used a non-zero file position!\n",
>> +               table->procname, current->comm);
>>         pr_warn("This will not be supported in the future.\n");
>>         pr_warn("To silence warning, set kernel.sysctl_writes_strict =
>> -1\n");
>>   }
>
>
> Why is this patch part of the test patch?

Oops, thanks. Yeah, that snuck into the wrong patch.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v3 0/4] sysctl: fix incorrect write position handling

[Andrew Morton]

On Thu,  1 May 2014 14:26:33 -0700 Kees Cook <keescook@chromium.org> wrote:

> When writing to a sysctl string, each write, regardless of VFS position,
> began writing the string from the start. This meant the contents of
> the last write to the sysctl controlled the string contents instead of
> the first.
> 
> This misbehavior was featured in an exploit against Chrome OS. While it's
> not in itself a vulnerability, it's a weirdness that isn't on the mind
> of most auditors: "This filter looks correct, the first line written
> would not be meaningful to sysctl" doesn't apply here, since the size
> of the write and the contents of the final write are what matter when
> writing to sysctls.
> 
> This adds the sysctl kernel.sysctl_writes_strict to control the write
> behavior. The default (0) reports when VFS position is non-0 on a write,
> but retains legacy behavior, -1 disables the warning, and 1 enables the
> position-respecting behavior.
> 

OK, let's try that.  I added this paragraph to the patchset's overall
changelog:

: The long-term plan here is to wait for userspace to be fixed in response
: to the new warning and to then switch the default kernel behavior to the
: new position-respecting behavior.

I'm thinking we should use pr_warn_once() in warn_sysctl_write()?  Otherwise
people will go and shut the thing up permanently and we'll lose the benefits.

Re: [PATCH v3 0/4] sysctl: fix incorrect write position handling

[Kees Cook]

On Mon, May 5, 2014 at 3:00 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu,  1 May 2014 14:26:33 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> When writing to a sysctl string, each write, regardless of VFS position,
>> began writing the string from the start. This meant the contents of
>> the last write to the sysctl controlled the string contents instead of
>> the first.
>>
>> This misbehavior was featured in an exploit against Chrome OS. While it's
>> not in itself a vulnerability, it's a weirdness that isn't on the mind
>> of most auditors: "This filter looks correct, the first line written
>> would not be meaningful to sysctl" doesn't apply here, since the size
>> of the write and the contents of the final write are what matter when
>> writing to sysctls.
>>
>> This adds the sysctl kernel.sysctl_writes_strict to control the write
>> behavior. The default (0) reports when VFS position is non-0 on a write,
>> but retains legacy behavior, -1 disables the warning, and 1 enables the
>> position-respecting behavior.
>>
>
> OK, let's try that.  I added this paragraph to the patchset's overall
> changelog:
>
> : The long-term plan here is to wait for userspace to be fixed in response
> : to the new warning and to then switch the default kernel behavior to the
> : new position-respecting behavior.

Great, thanks!

> I'm thinking we should use pr_warn_once() in warn_sysctl_write()?  Otherwise
> people will go and shut the thing up permanently and we'll lose the benefits.

I was worried we'd miss different processed tripping it later. On the
other hand, I didn't like the idea of being able to spam dmesg. Do you
want me to send a patch to replace that with pr_warn_once()?

-Kees

-- 
Kees Cook
Chrome OS Security