[PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()

[PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()

[Zev Weiss]

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

[PATCH 1/2] test_sysctl: add tests for >32-bit values written to 32-bit integers

[Zev Weiss]

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

[PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions

[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

Re: [PATCH 2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions

[Luis Chamberlain]

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

[PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()

[Zev Weiss]

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

[PATCH v2 1/3] test_sysctl: add tests for >32-bit values written to 32-bit integers

[Zev Weiss]

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

[PATCH v2 2/3] kernel/sysctl.c: add missing range check in do_proc_dointvec_minmax_conv

[Zev Weiss]

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

[PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions

[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

Re: [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()

[Luis Chamberlain]

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

Re: [PATCH v2 0/3] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()

[Zev Weiss]

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

[PATCH v2 3/3] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions

[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

Re: [PATCH 0/2] sysctl: fix range-checking in do_proc_dointvec_minmax_conv()

[Zev Weiss]

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
>