Linux-Fsdevel Archive on lore.kernel.org
 help / Atom feed
* [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()
@ 2018-12-27 11:12 Zev Weiss
  2018-12-27 11:12 ` [PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Zev Weiss @ 2018-12-27 11:12 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook; +Cc: linux-kernel, linux-fsdevel

Hello,

After being left with an unusable system after a typo executing
something like 'echo $((1<<24)) > /proc/sys/vm/max_map_count', I found
that do_proc_dointvec_minmax_conv() was missing a check to ensure that
the converted value actually fits in an int.

The first of the following patches enhances the sysctl selftest such
that it detects this problem; the second fixes it (wasn't entirely
sure if this would meet the criteria of something that should be sent
to -stable; input welcome).


Thanks,
Zev Weiss

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers
  2018-12-27 11:12 [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
@ 2018-12-27 11:12 ` Zev Weiss
  2018-12-27 11:12 ` [PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
  2019-02-05 16:23 ` [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
  2 siblings, 0 replies; 13+ messages in thread
From: Zev Weiss @ 2018-12-27 11:12 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook
  Cc: linux-kernel, linux-fsdevel, Zev Weiss, Shuah Khan, linux-kselftest

At present this exposes a bug in do_proc_dointvec_minmax_conv() (it
fails to check for values that are too wide to fit in an int).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 tools/testing/selftests/sysctl/sysctl.sh | 37 ++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 584eb8ea780a..a7d0da25975c 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -290,6 +290,40 @@ run_numerictests()
 	test_rc
 }
 
+check_failure()
+{
+	echo -n "Testing that $1 fails as expected..."
+	reset_vals
+	TEST_STR="$1"
+	orig="$(cat $TARGET)"
+	echo -n "$TEST_STR" > $TARGET 2> /dev/null
+
+	# write should fail and $TARGET should retain its original value
+	if [ $? = 0 ] || [ "$(cat $TARGET)" != "$orig" ]; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+}
+
+run_wideint_tests()
+{
+	# check negative and positive 64-bit values, with and without
+	# bits set in the lower 31, and with and without bit 31 (sign
+	# bit of a 32-bit int) set.  None of these are representable
+	# in 32 bits, and hence all should fail.
+	check_failure 0x0000010000000000
+	check_failure 0x0000010080000000
+	check_failure 0x000001ff7fffffff
+	check_failure 0x000001ffffffffff
+	check_failure 0xffffffff7fffffff
+	check_failure 0xffffffffffffffff
+	check_failure 0xffffff0000000000
+	check_failure 0xffffff0080000000
+}
+
 # Your test must accept digits 3 and 4 to use this
 run_limit_digit()
 {
@@ -556,6 +590,7 @@ sysctl_test_0001()
 	TEST_STR=$(( $ORIG + 1 ))
 
 	run_numerictests
+	run_wideint_tests
 	run_limit_digit
 }
 
@@ -580,6 +615,7 @@ sysctl_test_0003()
 	TEST_STR=$(( $ORIG + 1 ))
 
 	run_numerictests
+	run_wideint_tests
 	run_limit_digit
 	run_limit_digit_int
 }
@@ -592,6 +628,7 @@ sysctl_test_0004()
 	TEST_STR=$(( $ORIG + 1 ))
 
 	run_numerictests
+	run_wideint_tests
 	run_limit_digit
 	run_limit_digit_uint
 }
-- 
2.20.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions
  2018-12-27 11:12 [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
  2018-12-27 11:12 ` [PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
@ 2018-12-27 11:12 ` Zev Weiss
  2019-02-06 19:58   ` Luis Chamberlain
  2019-02-05 16:23 ` [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
  2 siblings, 1 reply; 13+ messages in thread
From: Zev Weiss @ 2018-12-27 11:12 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook; +Cc: linux-kernel, linux-fsdevel, Zev Weiss

do_proc_do[u]intvec_minmax_conv() had included open-coded versions of
do_proc_do[u]intvec_conv(), though the signed one omitted the check
that the value is in [INT_MIN, INT_MAX].  Rather than increase the
duplication further by copying the additional check, we can instead
refactor both to be defined in terms of their non-bounded counterparts
(plus the added check).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 kernel/sysctl.c | 50 ++++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 5fc724e4e454..4a0261d32401 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2562,23 +2562,26 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 					int *valp,
 					int write, void *data)
 {
+	int tmp, ret;
 	struct do_proc_dointvec_minmax_conv_param *param = data;
+
+	/*
+	 * First write to a temporary local int so we can bounds-check it
+	 * before touching *valp.
+	 */
+	int *ip = write ? &tmp : valp;
+
+	ret = do_proc_dointvec_conv(negp, lvalp, ip, write, data);
+	if (ret)
+		return ret;
+
 	if (write) {
-		int val = *negp ? -*lvalp : *lvalp;
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
 			return -EINVAL;
-		*valp = val;
-	} else {
-		int val = *valp;
-		if (val < 0) {
-			*negp = true;
-			*lvalp = -(unsigned long)val;
-		} else {
-			*negp = false;
-			*lvalp = (unsigned long)val;
-		}
+		*valp = tmp;
 	}
+
 	return 0;
 }
 
@@ -2627,22 +2630,23 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 					 unsigned int *valp,
 					 int write, void *data)
 {
+	int ret;
+	unsigned int tmp;
 	struct do_proc_douintvec_minmax_conv_param *param = data;
 
-	if (write) {
-		unsigned int val = *lvalp;
+	/* write via temporary local uint for bounds-checking */
+	unsigned int *up = write ? &tmp : valp;
 
-		if (*lvalp > UINT_MAX)
-			return -EINVAL;
+	ret = do_proc_douintvec_conv(lvalp, up, write, data);
+	if (ret)
+		return ret;
 
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+	if (write) {
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
 			return -ERANGE;
 
-		*valp = val;
-	} else {
-		unsigned int val = *valp;
-		*lvalp = (unsigned long) val;
+		*valp = tmp;
 	}
 
 	return 0;
-- 
2.20.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()
  2018-12-27 11:12 [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
  2018-12-27 11:12 ` [PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
  2018-12-27 11:12 ` [PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
@ 2019-02-05 16:23 ` Zev Weiss
  2 siblings, 0 replies; 13+ messages in thread
From: Zev Weiss @ 2019-02-05 16:23 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook; +Cc: linux-kernel, linux-fsdevel

Ping...any thoughts on these patches?  (May have slipped through the 
cracks over people's holiday vacations I'd guess.)

Thanks,
Zev

On Thu, Dec 27, 2018 at 05:12:28AM CST, Zev Weiss wrote:
>Hello,
>
>After being left with an unusable system after a typo executing
>something like 'echo $((1<<24)) > /proc/sys/vm/max_map_count', I found
>that do_proc_dointvec_minmax_conv() was missing a check to ensure that
>the converted value actually fits in an int.
>
>The first of the following patches enhances the sysctl selftest such
>that it detects this problem; the second fixes it (wasn't entirely
>sure if this would meet the criteria of something that should be sent
>to -stable; input welcome).
>
>
>Thanks,
>Zev Weiss
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions
  2018-12-27 11:12 ` [PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
@ 2019-02-06 19:58   ` Luis Chamberlain
  2019-02-07 12:34     ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
  0 siblings, 1 reply; 13+ messages in thread
From: Luis Chamberlain @ 2019-02-06 19:58 UTC (permalink / raw)
  To: Zev Weiss
  Cc: Kees Cook, linux-kernel, linux-fsdevel, akpm, yzaikin, brendanhiggins

On Thu, Dec 27, 2018 at 05:12:30AM -0600, Zev Weiss wrote:
> do_proc_do[u]intvec_minmax_conv() had included open-coded versions of
> do_proc_do[u]intvec_conv(), though the signed one omitted the check
> that the value is in [INT_MIN, INT_MAX].  Rather than increase the
> duplication further by copying the additional check, we can instead
> refactor both to be defined in terms of their non-bounded counterparts
> (plus the added check).

The code below looks fine, however it is a rather intrusive check.
Let's isntead open code the new bound check and Cc stable, and then
after we can get creative with the wrapper use.

Can you confirm the open coded version fixes the issues, and then
the other change does not regress? If you can include an annotation
as to since *when* this was broken by annotating it on your CC stable
note it would be useful for stable maintainers. Likewise if you can
add a respective Fixes: tag that would be appreciated if you can easily
find it.

The stable tag annotation can be placed on top of all the tags, for
instance if the first broken commit was in v4.1 then:

	Cc: <stable@vger.kernel.org> # v4.1+

Thanks for the fix and expanding on the tests!

  Luis

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()
  2019-02-06 19:58   ` Luis Chamberlain
@ 2019-02-07 12:34     ` Zev Weiss
  2019-02-07 12:34       ` [PATCH v2 1/3] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
                         ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Zev Weiss @ 2019-02-07 12:34 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook
  Cc: linux-kernel, linux-fsdevel, Andrew Morton, yzaikin, brendanhiggins

Hello,

After being left with an unusable system after a typo executing
something like 'echo $((1<<24)) > /proc/sys/vm/max_map_count', I found
that do_proc_dointvec_minmax_conv() was missing a check to ensure that
the converted value actually fits in an int.

The first of the following patches enhances the sysctl selftest such
that it detects this problem; the second provides a minimal fix
(suitable for -stable) such that the selftest passes.  The third patch
then performs a more thorough refactoring to eliminate the code
duplication that led to the bug in the first place (maintaining the
passing status of the selftest).


Changes in v2:
 - Rearranged selftest to also test negative values and provide more
   info in comments
 - Added intermediate patch as a minimal fix for -stable without the
   refactoring


Thanks,
Zev Weiss



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/3] test_sysctl: add tests for >32-bit values written to 32-bit integers
  2019-02-07 12:34     ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
@ 2019-02-07 12:34       ` Zev Weiss
  2019-02-07 12:34       ` [PATCH v2 2/3] kernel/sysctl.c: add missing range check in do_proc_dointvec_minmax_conv Zev Weiss
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Zev Weiss @ 2019-02-07 12:34 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook
  Cc: linux-kernel, linux-fsdevel, Andrew Morton, yzaikin,
	brendanhiggins, Zev Weiss, Shuah Khan, linux-kselftest

At present this exposes a bug in do_proc_dointvec_minmax_conv() (it
fails to check for values that are too wide to fit in an int).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 tools/testing/selftests/sysctl/sysctl.sh | 55 ++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 584eb8ea780a..780ce7123374 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -290,6 +290,58 @@ run_numerictests()
 	test_rc
 }
 
+check_failure()
+{
+	echo -n "Testing that $1 fails as expected..."
+	reset_vals
+	TEST_STR="$1"
+	orig="$(cat $TARGET)"
+	echo -n "$TEST_STR" > $TARGET 2> /dev/null
+
+	# write should fail and $TARGET should retain its original value
+	if [ $? = 0 ] || [ "$(cat $TARGET)" != "$orig" ]; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+}
+
+run_wideint_tests()
+{
+	# sysctl conversion functions receive a boolean sign and ulong
+	# magnitude; here we list the magnitudes we want to test (each of
+	# which will be tested in both positive and negative forms).  Since
+	# none of these values fit in 32 bits, writing them to an int- or
+	# uint-typed sysctl should fail.
+	local magnitudes=(
+		# common boundary-condition values (zero, +1, -1, INT_MIN,
+		# and INT_MAX respectively) if truncated to lower 32 bits
+		# (potential for being falsely deemed in range)
+		0x0000000100000000
+		0x0000000100000001
+		0x00000001ffffffff
+		0x0000000180000000
+		0x000000017fffffff
+
+		# these look like negatives, but without a leading '-' are
+		# actually large positives (should be rejected as above
+		# despite being zero/+1/-1/INT_MIN/INT_MAX in the lower 32)
+		0xffffffff00000000
+		0xffffffff00000001
+		0xffffffffffffffff
+		0xffffffff80000000
+		0xffffffff7fffffff
+	)
+
+	for sign in '' '-'; do
+		for mag in "${magnitudes[@]}"; do
+			check_failure "${sign}${mag}"
+		done
+	done
+}
+
 # Your test must accept digits 3 and 4 to use this
 run_limit_digit()
 {
@@ -556,6 +608,7 @@ sysctl_test_0001()
 	TEST_STR=$(( $ORIG + 1 ))
 
 	run_numerictests
+	run_wideint_tests
 	run_limit_digit
 }
 
@@ -580,6 +633,7 @@ sysctl_test_0003()
 	TEST_STR=$(( $ORIG + 1 ))
 
 	run_numerictests
+	run_wideint_tests
 	run_limit_digit
 	run_limit_digit_int
 }
@@ -592,6 +646,7 @@ sysctl_test_0004()
 	TEST_STR=$(( $ORIG + 1 ))
 
 	run_numerictests
+	run_wideint_tests
 	run_limit_digit
 	run_limit_digit_uint
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 2/3] kernel/sysctl.c: add missing range check in do_proc_dointvec_minmax_conv
  2019-02-07 12:34     ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
  2019-02-07 12:34       ` [PATCH v2 1/3] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
@ 2019-02-07 12:34       ` Zev Weiss
  2019-02-07 12:34       ` [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
                         ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Zev Weiss @ 2019-02-07 12:34 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook
  Cc: linux-kernel, linux-fsdevel, Andrew Morton, yzaikin,
	brendanhiggins, Zev Weiss, stable

This bug has apparently existed since the introduction of this function
in the pre-git era (4500e91754d3 in Thomas Gleixner's history.git,
"[NET]: Add proc_dointvec_userhz_jiffies, use it for proper handling of
neighbour sysctls.").  As a minimal fix we can simply duplicate the
corresponding check in do_proc_dointvec_conv().

Cc: <stable@vger.kernel.org> # v2.6.2+
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 kernel/sysctl.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 5fc724e4e454..a71c4b3935bc 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2564,7 +2564,16 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 {
 	struct do_proc_dointvec_minmax_conv_param *param = data;
 	if (write) {
-		int val = *negp ? -*lvalp : *lvalp;
+		int val;
+		if (*negp) {
+			if (*lvalp > (unsigned long) INT_MAX + 1)
+				return -EINVAL;
+			val = -*lvalp;
+		} else {
+			if (*lvalp > (unsigned long) INT_MAX)
+				return -EINVAL;
+			val = *lvalp;
+		}
 		if ((param->min && *param->min > val) ||
 		    (param->max && *param->max < val))
 			return -EINVAL;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions
  2019-02-07 12:34     ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
  2019-02-07 12:34       ` [PATCH v2 1/3] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
  2019-02-07 12:34       ` [PATCH v2 2/3] kernel/sysctl.c: add missing range check in do_proc_dointvec_minmax_conv Zev Weiss
@ 2019-02-07 12:34       ` Zev Weiss
  2019-02-07 15:51       ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Luis Chamberlain
  2019-02-07 16:51       ` [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
  4 siblings, 0 replies; 13+ messages in thread
From: Zev Weiss @ 2019-02-07 12:34 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook
  Cc: linux-kernel, linux-fsdevel, Andrew Morton, yzaikin,
	brendanhiggins, Zev Weiss

do_proc_do[u]intvec_minmax_conv() had included open-coded versions of
do_proc_do[u]intvec_conv(); the duplication led to buggy inconsistencies
(missing range checks).  To reduce the likelihood of such problems in
the future, we can instead refactor both to be defined in terms of their
non-bounded counterparts (plus the added check).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 kernel/sysctl.c | 59 ++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index a71c4b3935bc..4b6bce36737e 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2562,32 +2562,26 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 					int *valp,
 					int write, void *data)
 {
+	int tmp, ret;
 	struct do_proc_dointvec_minmax_conv_param *param = data;
+
+	/*
+	 * If writing, first do so via a temporary local int so we can
+	 * bounds-check it before touching *valp.
+	 */
+	int *ip = write ? &tmp : valp;
+
+	ret = do_proc_dointvec_conv(negp, lvalp, ip, write, data);
+	if (ret)
+		return ret;
+
 	if (write) {
-		int val;
-		if (*negp) {
-			if (*lvalp > (unsigned long) INT_MAX + 1)
-				return -EINVAL;
-			val = -*lvalp;
-		} else {
-			if (*lvalp > (unsigned long) INT_MAX)
-				return -EINVAL;
-			val = *lvalp;
-		}
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
 			return -EINVAL;
-		*valp = val;
-	} else {
-		int val = *valp;
-		if (val < 0) {
-			*negp = true;
-			*lvalp = -(unsigned long)val;
-		} else {
-			*negp = false;
-			*lvalp = (unsigned long)val;
-		}
+		*valp = tmp;
 	}
+
 	return 0;
 }
 
@@ -2636,22 +2630,23 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 					 unsigned int *valp,
 					 int write, void *data)
 {
+	int ret;
+	unsigned int tmp;
 	struct do_proc_douintvec_minmax_conv_param *param = data;
 
-	if (write) {
-		unsigned int val = *lvalp;
+	/* write via temporary local uint for bounds-checking */
+	unsigned int *up = write ? &tmp : valp;
 
-		if (*lvalp > UINT_MAX)
-			return -EINVAL;
+	ret = do_proc_douintvec_conv(lvalp, up, write, data);
+	if (ret)
+		return ret;
 
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+	if (write) {
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
 			return -ERANGE;
 
-		*valp = val;
-	} else {
-		unsigned int val = *valp;
-		*lvalp = (unsigned long) val;
+		*valp = tmp;
 	}
 
 	return 0;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()
  2019-02-07 12:34     ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
                         ` (2 preceding siblings ...)
  2019-02-07 12:34       ` [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
@ 2019-02-07 15:51       ` Luis Chamberlain
  2019-02-07 16:54         ` Zev Weiss
  2019-02-07 16:51       ` [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
  4 siblings, 1 reply; 13+ messages in thread
From: Luis Chamberlain @ 2019-02-07 15:51 UTC (permalink / raw)
  To: Zev Weiss
  Cc: Kees Cook, linux-kernel, linux-fsdevel, Andrew Morton, yzaikin,
	brendanhiggins

On Thu, Feb 07, 2019 at 06:34:23AM -0600, Zev Weiss wrote:
> Hello,
> 
> After being left with an unusable system after a typo executing
> something like 'echo $((1<<24)) > /proc/sys/vm/max_map_count', I found
> that do_proc_dointvec_minmax_conv() was missing a check to ensure that
> the converted value actually fits in an int.
> 
> The first of the following patches enhances the sysctl selftest such
> that it detects this problem; the second provides a minimal fix
> (suitable for -stable) such that the selftest passes.  The third patch
> then performs a more thorough refactoring to eliminate the code
> duplication that led to the bug in the first place (maintaining the
> passing status of the selftest).
> 
> 
> Changes in v2:
>  - Rearranged selftest to also test negative values and provide more
>    info in comments
>  - Added intermediate patch as a minimal fix for -stable without the
>    refactoring

Thanks! For some reason I got all except the last patch, patch #3.
Can you bounce me and others a copy?

  Luis

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions
  2019-02-07 12:34     ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
                         ` (3 preceding siblings ...)
  2019-02-07 15:51       ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Luis Chamberlain
@ 2019-02-07 16:51       ` Zev Weiss
  4 siblings, 0 replies; 13+ messages in thread
From: Zev Weiss @ 2019-02-07 16:51 UTC (permalink / raw)
  To: Luis Chamberlain, Kees Cook
  Cc: linux-kernel, linux-fsdevel, Andrew Morton, yzaikin,
	brendanhiggins, Zev Weiss

do_proc_do[u]intvec_minmax_conv() had included open-coded versions of
do_proc_do[u]intvec_conv(); the duplication led to buggy inconsistencies
(missing range checks).  To reduce the likelihood of such problems in
the future, we can instead refactor both to be defined in terms of their
non-bounded counterparts (plus the added check).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
  kernel/sysctl.c | 59 ++++++++++++++++++++++---------------------------
  1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index a71c4b3935bc..4b6bce36737e 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2562,32 +2562,26 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
  					int *valp,
  					int write, void *data)
  {
+	int tmp, ret;
  	struct do_proc_dointvec_minmax_conv_param *param = data;
+
+	/*
+	 * If writing, first do so via a temporary local int so we can
+	 * bounds-check it before touching *valp.
+	 */
+	int *ip = write ? &tmp : valp;
+
+	ret = do_proc_dointvec_conv(negp, lvalp, ip, write, data);
+	if (ret)
+		return ret;
+
  	if (write) {
-		int val;
-		if (*negp) {
-			if (*lvalp > (unsigned long) INT_MAX + 1)
-				return -EINVAL;
-			val = -*lvalp;
-		} else {
-			if (*lvalp > (unsigned long) INT_MAX)
-				return -EINVAL;
-			val = *lvalp;
-		}
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
  			return -EINVAL;
-		*valp = val;
-	} else {
-		int val = *valp;
-		if (val < 0) {
-			*negp = true;
-			*lvalp = -(unsigned long)val;
-		} else {
-			*negp = false;
-			*lvalp = (unsigned long)val;
-		}
+		*valp = tmp;
  	}
+
  	return 0;
  }
  
@@ -2636,22 +2630,23 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
  					 unsigned int *valp,
  					 int write, void *data)
  {
+	int ret;
+	unsigned int tmp;
  	struct do_proc_douintvec_minmax_conv_param *param = data;
  
-	if (write) {
-		unsigned int val = *lvalp;
+	/* write via temporary local uint for bounds-checking */
+	unsigned int *up = write ? &tmp : valp;
  
-		if (*lvalp > UINT_MAX)
-			return -EINVAL;
+	ret = do_proc_douintvec_conv(lvalp, up, write, data);
+	if (ret)
+		return ret;
  
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+	if (write) {
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
  			return -ERANGE;
  
-		*valp = val;
-	} else {
-		unsigned int val = *valp;
-		*lvalp = (unsigned long) val;
+		*valp = tmp;
  	}
  
  	return 0;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()
  2019-02-07 15:51       ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Luis Chamberlain
@ 2019-02-07 16:54         ` Zev Weiss
  0 siblings, 0 replies; 13+ messages in thread
From: Zev Weiss @ 2019-02-07 16:54 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: Kees Cook, linux-kernel, linux-fsdevel, Andrew Morton, yzaikin,
	brendanhiggins

On Thu, Feb 07, 2019 at 09:51:44AM CST, Luis Chamberlain wrote:
>On Thu, Feb 07, 2019 at 06:34:23AM -0600, Zev Weiss wrote:
>> Hello,
>>
>> After being left with an unusable system after a typo executing
>> something like 'echo $((1<<24)) > /proc/sys/vm/max_map_count', I found
>> that do_proc_dointvec_minmax_conv() was missing a check to ensure that
>> the converted value actually fits in an int.
>>
>> The first of the following patches enhances the sysctl selftest such
>> that it detects this problem; the second provides a minimal fix
>> (suitable for -stable) such that the selftest passes.  The third patch
>> then performs a more thorough refactoring to eliminate the code
>> duplication that led to the bug in the first place (maintaining the
>> passing status of the selftest).
>>
>>
>> Changes in v2:
>>  - Rearranged selftest to also test negative values and provide more
>>    info in comments
>>  - Added intermediate patch as a minimal fix for -stable without the
>>    refactoring
>
>Thanks! For some reason I got all except the last patch, patch #3.
>Can you bounce me and others a copy?
>
>  Luis

Hmm, odd -- it does seem like each time I use git-send-email I manage to 
find a new way to botch it up, but in this case it *looks* like my 
server logs indicate that one should have been sent properly as far as I 
can tell.  No matter, resent it manually anyway, hopefully it gets 
through this time...(apologies if anyone gets duplicate copies).


Zev


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers
@ 2019-02-06 19:52 Luis Chamberlain
  0 siblings, 0 replies; 13+ messages in thread
From: Luis Chamberlain @ 2019-02-06 19:52 UTC (permalink / raw)
  To: Zev Weiss
  Cc: Kees Cook, linux-kernel, linux-fsdevel, Shuah Khan,
	linux-kselftest, akpm, yzaikin, brendanhiggins

Thanks for the patches, please include akpm@linux-foundation.org in the
future, as we can merge the changes through Andrew as well.

Also please Cc yzaikin@google.com, brendanhiggins@google.com in follow
ups for now.  They are looking at the sysctl testing code as well.

Some feedback below:

In-Reply-To: <20181227111231.12912-2-zev@bewilderbeest.net>

On Thu, Dec 27, 2018 at 05:12:29AM -0600, Zev Weiss wrote:
> +run_wideint_tests()
> +{
> +	# check negative and positive 64-bit values, with and without
> +	# bits set in the lower 31, and with and without bit 31 (sign
> +	# bit of a 32-bit int) set.  None of these are representable
> +	# in 32 bits, and hence all should fail.
> +	check_failure 0x0000010000000000
> +	check_failure 0x0000010080000000
> +	check_failure 0x000001ff7fffffff
> +	check_failure 0x000001ffffffffff
> +	check_failure 0xffffffff7fffffff


> +	check_failure 0xffffffffffffffff

This s64 version of -1

> +	check_failure 0xffffff0000000000
> +	check_failure 0xffffff0080000000
> +}

It was still unclear from the comments and manually looking at the
values why they are clear candidates to always test from all respective
64-bit values. A comment per each would be useful.

  Luis

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-27 11:12 [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
2018-12-27 11:12 ` [PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
2018-12-27 11:12 ` [PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
2019-02-06 19:58   ` Luis Chamberlain
2019-02-07 12:34     ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
2019-02-07 12:34       ` [PATCH v2 1/3] test_sysctl: add tests for >32-bit values written to 32-bit integers Zev Weiss
2019-02-07 12:34       ` [PATCH v2 2/3] kernel/sysctl.c: add missing range check in do_proc_dointvec_minmax_conv Zev Weiss
2019-02-07 12:34       ` [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
2019-02-07 15:51       ` [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Luis Chamberlain
2019-02-07 16:54         ` Zev Weiss
2019-02-07 16:51       ` [PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions Zev Weiss
2019-02-05 16:23 ` [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv() Zev Weiss
2019-02-06 19:52 [PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers Luis Chamberlain

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org linux-fsdevel@archiver.kernel.org
	public-inbox-index linux-fsdevel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox