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