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

[PATCH v2 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 procfs.

For the paranoid, introduce CONFIG_PROC_SYSCTL_STRICT_WRITES to
change the behavior to track file position most strictly.

Thanks,

-Kees

[PATCH v2 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 v2 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.

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 v2 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.

This provides CONFIG_PROC_SYSCTL_STRICT_WRITES 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).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/proc/Kconfig |   17 +++++++++++++++++
 kernel/sysctl.c |   17 +++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 2183fcf41d59..4a93cf8b7b9f 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -62,6 +62,23 @@ config PROC_SYSCTL
 	  building a kernel for install/rescue disks or your system is very
 	  limited in memory.
 
+config PROC_SYSCTL_STRICT_WRITES
+	bool "Perform strict writes to /proc/sys entries" if EXPERT
+	depends on PROC_SYSCTL
+	default n
+	---help---
+	  When writing to sysctl entries in /proc, each write syscall
+	  is expected to contain the entire sysctl value. For strings
+	  this means that writes do not append, and the contents of the
+	  buffer will be whatever was written last, regardless of the
+	  file position.
+
+	  When PROC_SYSCTL_STRICT_WRITES is enabled, writes to numeric
+	  sysctl entries must always be at file position 0 and the value
+	  must be fully contained in the buffer sent to the write syscall.
+	  For strings, file position is respected, though anything past
+	  the max length of the sysctl buffer will be ignored.
+
 config PROC_PAGE_MONITOR
  	default y
 	depends on PROC_FS && MMU
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0e08103a69c8..83902ae3e4e6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1712,8 +1712,19 @@ static int _proc_do_string(char *data, int maxlen, int write,
 	}
 
 	if (write) {
+#ifdef CONFIG_PROC_SYSCTL_STRICT_WRITES
+		/* 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;
+#endif
 		*ppos += *lenp;
 		p = buffer;
 		while ((p - buffer) < *lenp && len < maxlen - 1) {
@@ -1948,6 +1959,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 		conv = do_proc_dointvec_conv;
 
 	if (write) {
+		if (IS_ENABLED(CONFIG_PROC_SYSCTL_STRICT_WRITES) && *ppos)
+			goto out;
 		if (left > PAGE_SIZE - 1)
 			left = PAGE_SIZE - 1;
 		page = __get_free_page(GFP_TEMPORARY);
@@ -2005,6 +2018,7 @@ free:
 			return err ? : -EINVAL;
 	}
 	*lenp -= left;
+out:
 	*ppos += *lenp;
 	return err;
 }
@@ -2197,6 +2211,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 	left = *lenp;
 
 	if (write) {
+		if (IS_ENABLED(CONFIG_PROC_SYSCTL_STRICT_WRITES) && *ppos)
+			goto out;
 		if (left > PAGE_SIZE - 1)
 			left = PAGE_SIZE - 1;
 		page = __get_free_page(GFP_TEMPORARY);
@@ -2252,6 +2268,7 @@ free:
 			return err ? : -EINVAL;
 	}
 	*lenp -= left;
+out:
 	*ppos += *lenp;
 	return err;
 }
-- 
1.7.9.5

[PATCH v2 4/4] test: validate CONFIG_PROC_SYSCTL_STRICT_WRITES

[Kees Cook]

This adds several behavioral tests to sysctl string and number writing
to detect unexpected cases that behaved differently when the option
CONFIG_PROC_SYSCTL_STRICT_WRITES was unset.

[ 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>
---
 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 ++++++++++++++++
 5 files changed, 216 insertions(+)
 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/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..3fc94dc83b44
--- /dev/null
+++ b/tools/testing/selftests/sysctl/Makefile
@@ -0,0 +1,19 @@
+# Makefile for sysctl selftests.
+# Expects CONFIG_PROC_SYSCTL_STRICT_WRITES=y.
+
+# 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 v2 1/4] sysctl: clean up char buffer arguments

[Andi Kleen]

BTW if you're worried about sysctl races you may also want to resurrect
https://lkml.org/lkml/2009/12/20/214

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

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

[Kees Cook]

On Thu, Apr 17, 2014 at 5:28 PM, Andi Kleen <ak@linux.intel.com> wrote:
> BTW if you're worried about sysctl races you may also want to resurrect
> https://lkml.org/lkml/2009/12/20/214

Yeah, this had bothered me too while reading the code. The only "good"
news is that the strings are always NULL terminated and only "old"
contents can ever leak. Still, it's kind of horrible that there is no
locking here. :)

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Andrew Morton]

On Thu, 17 Apr 2014 17:16:22 -0700 Kees Cook <keescook@chromium.org> wrote:

> 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.
> 
> This provides CONFIG_PROC_SYSCTL_STRICT_WRITES 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).

Adding a Kconfig knob to alter the behavior of procfs writes creeps me
out.  I wonder why.

- I doubt if many people have a sufficient amount of control over
  their entire systems to be able to confidently set
  CONFIG_PROC_SYSCTL_STRICT_WRITES.

- Software will be shipped which runs OK with one setting but breaks
  with the other setting.

So what to do?

I think we can *detect* this situation easily enough.  So some options are

a) change the behaviour and add code which detects when userspace is
   doing a write whose behaviour is now altered.  Print a warning.   Or

b) leave the behaviour as-is.  Add a detector which tells people
   "hey, your userspace is probably broken - please fix".  Wait N
   years.  Then alter the behaviour as in a).

In either case the detector should display current->comm, the procfs
pathname and the contents of the write, to aid people in hunting down
and fixing their userspace.

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

[Kees Cook]

On Mon, Apr 21, 2014 at 3:45 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 17 Apr 2014 17:16:22 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> 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.
>>
>> This provides CONFIG_PROC_SYSCTL_STRICT_WRITES 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).
>
> Adding a Kconfig knob to alter the behavior of procfs writes creeps me
> out.  I wonder why.
>
> - I doubt if many people have a sufficient amount of control over
>   their entire systems to be able to confidently set
>   CONFIG_PROC_SYSCTL_STRICT_WRITES.
>
> - Software will be shipped which runs OK with one setting but breaks
>   with the other setting.
>
> So what to do?
>
> I think we can *detect* this situation easily enough.  So some options are
>
> a) change the behaviour and add code which detects when userspace is
>    doing a write whose behaviour is now altered.  Print a warning.   Or
>
> b) leave the behaviour as-is.  Add a detector which tells people
>    "hey, your userspace is probably broken - please fix".  Wait N
>    years.  Then alter the behaviour as in a).
>
> In either case the detector should display current->comm, the procfs
> pathname and the contents of the write, to aid people in hunting down
> and fixing their userspace.

How about a tri-state sysctl (har har control sysctl behavior with a
sysctl) that defaults ("1") to existing behavior (to not break
anything) with a warning. Mode "2" uses new behavior, and mode "0"
uses existing behavior without a warning? Then we can wait N years and
switch the default to "2"?

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Andrew Morton]

On Mon, 21 Apr 2014 21:52:48 -0700 Kees Cook <keescook@chromium.org> wrote:

> >> This provides CONFIG_PROC_SYSCTL_STRICT_WRITES 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).
> >
> > Adding a Kconfig knob to alter the behavior of procfs writes creeps me
> > out.  I wonder why.
> >
> > - I doubt if many people have a sufficient amount of control over
> >   their entire systems to be able to confidently set
> >   CONFIG_PROC_SYSCTL_STRICT_WRITES.
> >
> > - Software will be shipped which runs OK with one setting but breaks
> >   with the other setting.
> >
> > So what to do?
> >
> > I think we can *detect* this situation easily enough.  So some options are
> >
> > a) change the behaviour and add code which detects when userspace is
> >    doing a write whose behaviour is now altered.  Print a warning.   Or
> >
> > b) leave the behaviour as-is.  Add a detector which tells people
> >    "hey, your userspace is probably broken - please fix".  Wait N
> >    years.  Then alter the behaviour as in a).
> >
> > In either case the detector should display current->comm, the procfs
> > pathname and the contents of the write, to aid people in hunting down
> > and fixing their userspace.
> 
> How about a tri-state sysctl (har har control sysctl behavior with a
> sysctl) that defaults ("1") to existing behavior (to not break
> anything) with a warning. Mode "2" uses new behavior, and mode "0"
> uses existing behavior without a warning? Then we can wait N years and
> switch the default to "2"?

Yes, I suppose that's more flexible.

I do have my doubts about whether we'll ever be able to change the
behaviour.  There will be soooo many random proc-pokers out there and
the amount of dusty-deck software will only increase over time.

I suppose the first thing to do is to get the warning in there and see
if we can get an understanding of how much code is likely to be
affected by the change.  Add "please email Kees" to the printk ;) I did
that once, many years ago.  I got a lot of email.  Didn't do it again.

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

[Kees Cook]

On Tue, Apr 22, 2014 at 11:11 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 21 Apr 2014 21:52:48 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> >> This provides CONFIG_PROC_SYSCTL_STRICT_WRITES 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).
>> >
>> > Adding a Kconfig knob to alter the behavior of procfs writes creeps me
>> > out.  I wonder why.
>> >
>> > - I doubt if many people have a sufficient amount of control over
>> >   their entire systems to be able to confidently set
>> >   CONFIG_PROC_SYSCTL_STRICT_WRITES.
>> >
>> > - Software will be shipped which runs OK with one setting but breaks
>> >   with the other setting.
>> >
>> > So what to do?
>> >
>> > I think we can *detect* this situation easily enough.  So some options are
>> >
>> > a) change the behaviour and add code which detects when userspace is
>> >    doing a write whose behaviour is now altered.  Print a warning.   Or
>> >
>> > b) leave the behaviour as-is.  Add a detector which tells people
>> >    "hey, your userspace is probably broken - please fix".  Wait N
>> >    years.  Then alter the behaviour as in a).
>> >
>> > In either case the detector should display current->comm, the procfs
>> > pathname and the contents of the write, to aid people in hunting down
>> > and fixing their userspace.
>>
>> How about a tri-state sysctl (har har control sysctl behavior with a
>> sysctl) that defaults ("1") to existing behavior (to not break
>> anything) with a warning. Mode "2" uses new behavior, and mode "0"
>> uses existing behavior without a warning? Then we can wait N years and
>> switch the default to "2"?
>
> Yes, I suppose that's more flexible.
>
> I do have my doubts about whether we'll ever be able to change the
> behaviour.  There will be soooo many random proc-pokers out there and
> the amount of dusty-deck software will only increase over time.

Yeah. Though at least in all the configurations I've tested, this
doesn't produce any hits:
$ sudo lsof -n | grep /proc/sys

It's by no means a definitive survey, but at least the trivial cases
aren't a problem.

> I suppose the first thing to do is to get the warning in there and see
> if we can get an understanding of how much code is likely to be
> affected by the change.  Add "please email Kees" to the printk ;) I did
> that once, many years ago.  I got a lot of email.  Didn't do it again.

Yeah, I was figuring putting the new sysctl knob name in the printk
would be more educational and less inbox-filling. :)

-Kees

-- 
Kees Cook
Chrome OS Security